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

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.

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.