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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
