1. Patchew
  2. Libvirt
  3. [PATCH for 7.0.0 v1 00/26] Introduce virtio memory support
  4. View patch

[PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()

Michal Privoznik posted 26 patches 5 years, 2 months ago
There is a newer version of this series
[PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Michal Privoznik 5 years, 2 months ago 
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain_address.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 2788dc7fb3..d872f75b38 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
     if (qemuDomainIsS390CCW(def) &&
         virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
         if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
-            qemuDomainPrimeVfioDeviceAddresses(
-                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
-        qemuDomainPrimeVirtioDeviceAddresses(
-            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
+            qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
+
+        qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
 
         if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
             goto cleanup;
 
     } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
         /* deal with legacy virtio-s390 */
-        qemuDomainPrimeVirtioDeviceAddresses(
-            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390);
+        qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390);
     }
 
     ret = 0;
-- 
2.26.2
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Thomas Huth 5 years, 2 months ago 
On 27/11/2020 16.02, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain_address.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 2788dc7fb3..d872f75b38 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
>      if (qemuDomainIsS390CCW(def) &&
>          virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
>          if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
> -            qemuDomainPrimeVfioDeviceAddresses(
> -                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> -        qemuDomainPrimeVirtioDeviceAddresses(
> -            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> +            qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);

Looks fine to me, but docs/coding-style.rst still suggest to format code
with "indent -l75", so is this really the right thing to do here?

> +        qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>  
>          if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
>              goto cleanup;
>  
>      } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {

Not related to your patch, but an idea for a future clean-up: That
QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
ccw) machine that has been removed in QEMU v2.6 already:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d

IIRC, that machine was already considered as deprecated since a couple of
earlier QEMU releases, so I really doubt that anybody is still using that in
production today.

Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
removed from libvirt nowadays.

 Thomas
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Michal Privoznik 5 years, 2 months ago 
On 11/30/20 10:38 AM, Thomas Huth wrote:
> On 27/11/2020 16.02, Michal Privoznik wrote:
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/qemu/qemu_domain_address.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index 2788dc7fb3..d872f75b38 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
>>       if (qemuDomainIsS390CCW(def) &&
>>           virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
>>           if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
>> -            qemuDomainPrimeVfioDeviceAddresses(
>> -                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>> -        qemuDomainPrimeVirtioDeviceAddresses(
>> -            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>> +            qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> 
> Looks fine to me, but docs/coding-style.rst still suggest to format code
> with "indent -l75", so is this really the right thing to do here?

It's true that we have 80 characters limit, but that is more of a soft 
limit and common sense should be used. Personally, I find

    func(
arg1, arg2
);

worse than exceeding that 80 char rule. My common sense tells me that 
the rule tries to avoid the following pattern (among others):

   func(arg1, arg2, ...., very_long_list_of_arguments, which, could, 
easily, go_on_multiple_lines, but, didnt);


> 
>> +        qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>   
>>           if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
>>               goto cleanup;
>>   
>>       } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> 
> Not related to your patch, but an idea for a future clean-up: That
> QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
> ccw) machine that has been removed in QEMU v2.6 already:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
> 
> IIRC, that machine was already considered as deprecated since a couple of
> earlier QEMU releases, so I really doubt that anybody is still using that in
> production today.
> 
> Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
> removed from libvirt nowadays.

That is even better idea. But currently libvirt supports QEMU-1.5.0 and 
newer. So I think we shouldn't remove that until the minimum version is 
bumped even though we think feature has no users.

https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4

Although, it might be about time to look again what is the oldest QEMU 
we need to support.

Michal
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Daniel P. Berrangé 5 years, 2 months ago 
On Mon, Nov 30, 2020 at 11:18:20AM +0100, Michal Privoznik wrote:
> On 11/30/20 10:38 AM, Thomas Huth wrote:
> > On 27/11/2020 16.02, Michal Privoznik wrote:
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   src/qemu/qemu_domain_address.c | 10 ++++------
> > >   1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> > > index 2788dc7fb3..d872f75b38 100644
> > > --- a/src/qemu/qemu_domain_address.c
> > > +++ b/src/qemu/qemu_domain_address.c
> > > @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
> > >       if (qemuDomainIsS390CCW(def) &&
> > >           virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
> > >           if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
> > > -            qemuDomainPrimeVfioDeviceAddresses(
> > > -                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > > -        qemuDomainPrimeVirtioDeviceAddresses(
> > > -            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > > +            qemuDomainPrimeVfioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > 
> > Looks fine to me, but docs/coding-style.rst still suggest to format code
> > with "indent -l75", so is this really the right thing to do here?
> 
> It's true that we have 80 characters limit, but that is more of a soft limit
> and common sense should be used. Personally, I find
> 
>    func(
> arg1, arg2
> );
> 
> worse than exceeding that 80 char rule. My common sense tells me that the
> rule tries to avoid the following pattern (among others):
> 
>   func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily,
> go_on_multiple_lines, but, didnt);
> 
> 
> > 
> > > +        qemuDomainPrimeVirtioDeviceAddresses(def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
> > >           if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
> > >               goto cleanup;
> > >       } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> > 
> > Not related to your patch, but an idea for a future clean-up: That
> > QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
> > ccw) machine that has been removed in QEMU v2.6 already:
> > 
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
> > 
> > IIRC, that machine was already considered as deprecated since a couple of
> > earlier QEMU releases, so I really doubt that anybody is still using that in
> > production today.
> > 
> > Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
> > removed from libvirt nowadays.
> 
> That is even better idea. But currently libvirt supports QEMU-1.5.0 and
> newer. So I think we shouldn't remove that until the minimum version is
> bumped even though we think feature has no users.
> 
> https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
> 
> Although, it might be about time to look again what is the oldest QEMU we
> need to support.

It was set due to the base RHEL-7 QEMU version.  RHEL-7 will fall out of
our support matrix at start of May 2021, so in a few months time we'll
have a massive QEMU version bump we can do, along with a more general
cleanup of RHEL-7 vintage cruft.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Cornelia Huck 5 years, 2 months ago 
On Mon, 30 Nov 2020 11:18:20 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 11/30/20 10:38 AM, Thomas Huth wrote:
> > On 27/11/2020 16.02, Michal Privoznik wrote:  
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>   src/qemu/qemu_domain_address.c | 10 ++++------
> >>   1 file changed, 4 insertions(+), 6 deletions(-)

> >>       } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {  
> > 
> > Not related to your patch, but an idea for a future clean-up: That
> > QEMU_CAPS_VIRTIO_S390 seems to belong to the ancient "s390-virtio" (without
> > ccw) machine that has been removed in QEMU v2.6 already:
> > 
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=7b3fdbd9a82
> > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3538fb6f89d
> > 
> > IIRC, that machine was already considered as deprecated since a couple of
> > earlier QEMU releases, so I really doubt that anybody is still using that in
> > production today.
> > 
> > Thus I think that all code related to QEMU_CAPS_VIRTIO_S390 could likely be
> > removed from libvirt nowadays.  
> 
> That is even better idea. But currently libvirt supports QEMU-1.5.0 and 
> newer. So I think we shouldn't remove that until the minimum version is 
> bumped even though we think feature has no users.
> 
> https://gitlab.com/libvirt/libvirt/-/commit/c1bc9c662b4
> 
> Although, it might be about time to look again what is the oldest QEMU 
> we need to support.

Would be great if you could bump it enough to get rid of the old
virtio-s390 transport :)

FWIW, virtio-ccw was introduced in QEMU 1.4, and became the default
with QEMU 2.4, although it had supplanted virtio-s390 well before that.
What are the criteria for possibly removing support for a feature in
libvirt: that nobody would use it in practice, or that nobody would be
able to use it?
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Thomas Huth 5 years, 2 months ago 
On 30/11/2020 11.18, Michal Privoznik wrote:
> On 11/30/20 10:38 AM, Thomas Huth wrote:
>> On 27/11/2020 16.02, Michal Privoznik wrote:
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>   src/qemu/qemu_domain_address.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>>> index 2788dc7fb3..d872f75b38 100644
>>> --- a/src/qemu/qemu_domain_address.c
>>> +++ b/src/qemu/qemu_domain_address.c
>>> @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
>>>       if (qemuDomainIsS390CCW(def) &&
>>>           virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
>>>           if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
>>> -            qemuDomainPrimeVfioDeviceAddresses(
>>> -                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>> -        qemuDomainPrimeVirtioDeviceAddresses(
>>> -            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>> +            qemuDomainPrimeVfioDeviceAddresses(def,
>>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>
>> Looks fine to me, but docs/coding-style.rst still suggest to format code
>> with "indent -l75", so is this really the right thing to do here?
> 
> It's true that we have 80 characters limit, but that is more of a soft limit
> and common sense should be used. Personally, I find
> 
>    func(
> arg1, arg2
> );
> 
> worse than exceeding that 80 char rule. My common sense tells me that the
> rule tries to avoid the following pattern (among others):
> 
>   func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily,
> go_on_multiple_lines, but, didnt);

Fair point, but then this should IMHO be reflected in the coding-style doc
first. Otherwise the next contributor to this file might simply undo your
change to fit everything again into the 80 (or even 75) columns limit...?

 Thomas
Re: [PATCH v1 05/26] qemu_domain_address: Reformat qemuDomainAssignS390Addresses()
Posted by Michal Privoznik 5 years, 2 months ago 
On 11/30/20 12:48 PM, Thomas Huth wrote:
> On 30/11/2020 11.18, Michal Privoznik wrote:
>> On 11/30/20 10:38 AM, Thomas Huth wrote:
>>> On 27/11/2020 16.02, Michal Privoznik wrote:
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>    src/qemu/qemu_domain_address.c | 10 ++++------
>>>>    1 file changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>>>> index 2788dc7fb3..d872f75b38 100644
>>>> --- a/src/qemu/qemu_domain_address.c
>>>> +++ b/src/qemu/qemu_domain_address.c
>>>> @@ -408,18 +408,16 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
>>>>        if (qemuDomainIsS390CCW(def) &&
>>>>            virQEMUCapsGet(qemuCaps, QEMU_CAPS_CCW)) {
>>>>            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW))
>>>> -            qemuDomainPrimeVfioDeviceAddresses(
>>>> -                def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>>> -        qemuDomainPrimeVirtioDeviceAddresses(
>>>> -            def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>>> +            qemuDomainPrimeVfioDeviceAddresses(def,
>>>> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
>>>
>>> Looks fine to me, but docs/coding-style.rst still suggest to format code
>>> with "indent -l75", so is this really the right thing to do here?
>>
>> It's true that we have 80 characters limit, but that is more of a soft limit
>> and common sense should be used. Personally, I find
>>
>>     func(
>> arg1, arg2
>> );
>>
>> worse than exceeding that 80 char rule. My common sense tells me that the
>> rule tries to avoid the following pattern (among others):
>>
>>    func(arg1, arg2, ...., very_long_list_of_arguments, which, could, easily,
>> go_on_multiple_lines, but, didnt);
> 
> Fair point, but then this should IMHO be reflected in the coding-style doc
> first. Otherwise the next contributor to this file might simply undo your
> change to fit everything again into the 80 (or even 75) columns limit...?

I can try. But these coding style rules are always hard to get right. 
There is always some counter example. Well, let me try.

Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.