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

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(a)redhat.com&gt; > > --- > > 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.