[libvirt] [PATCH v2 1/5] qemu: change the logic of setting default USB controller

[Pavel Hrdina]

On Thu, Apr 27, 2017 at 08:14:17PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-04-27 at 18:09 +0200, Pavel Hrdina wrote:
> > @@ -3200,11 +3200,17 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont,
> >               * when the relevant device is not available.
> >               *
> >               * See qemuBuildControllerDevCommandLine() */
> > -            if (ARCH_IS_S390(def->os.arch) &&
> > -                cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > -                /* set the default USB model to none for s390 unless an
> > -                 * address is found */
> > -                cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> > +
> > +            /* Default USB controller is piix3-uhci if available. */
> > +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI))
> > +                cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> > +
> > +            if (ARCH_IS_S390(def->os.arch)) {
> > +                if (cont->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > +                    /* set the default USB model to none for s390 unless an
> > +                     * address is found */
> > +                    cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE;
> > +                }
> 
> You should add an else branch where you unset cont->model
> explicitly, like you do for ppc64 below, otherwise s390
> guests will start being assigned piix3-uhci controllers in
> some situations.

That would change the output of this code, this patch preserver
the ouput, just changes the logic to be easier to follow.

> We don't have checks for that in our test suite, and it's
> probably not going to be much of an issue in the real world
> at least as far as downstream distros are concerned, but it
> should be avoided nonetheless.
> 
> 
> Overall I'm not convinced this is any more readable than
> what we had before or that it makes subsequent changes
> any easier, so I'd prefer if you'd just drop this patch.

The issue with original code is that the if else-if else condition
is not consistent.

The first if checks S390 and address type together, however the second
else-if checks only for PPC64, the capability checks are inside that block.

So if arch is S390 but the address type is different from
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE it continues to the last else block and
sets the model to piix3-uhci it it's available.

The second else-if for PPC64 doesn't fallback to piix3-uhci it the
architecture is PPC64 but none of the capabilities from that block
are set.

This patch changes the logic and also makes the if else-if more clearer
so the first check is only for architecture and after that in each block
we test for capabilities.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170427/44a8cd7f/attachment-0001.sig>