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@xxxxxxxxxx>
> > > ---
> > > 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 :|
