On 02/13/2018 01:43 PM, Daniel P. Berrangé wrote:
> GCC8 identified a critical flaw in the QEMU post-parse callback
How does it differentiate between this and the myriad of other places
that we are typecasting an int to an enum when writing a switch
statement? Try this for examples:

    grep 'switch ((vir[A-Za-z]*)' src/*/*.c


> > > They switch() based on the virDomainControllDef 'model', carefully >
listing every single enum case..... but no default statement.
When we first started doing that (typecasting  ints in switch statements
to enums in order to remove the default: and thus be notified when a new
value was added to the enum) I had an uneasy feeling about it - you can
claim that int is only ever set to valid enum values all you want, it
doesn't make it true (as this example proves) - typecasting the int and
only providing cases for valid enum values is akin to shoving your
fingers in your ears and shouting "LALALALALALALALALA!!!" while your
doctor tries to tell you you should exercise and cut down on the carbs.

> > > Unfortunately it is valid for "model" to have the valid "-1" which >
does not map to any enum constant. So the lack of any "default" >
statement, means when dealing with the USB controller we mistakenly >
fall through to the IDE controller code. > > The SCSI controller
simililarly falls through to the virtio serial > controller case. > > I
tried just adding the obvious default case:
... because the preceding typecast prevents you from specifying -1
directly :-/

> > > diff --git a/src/qemu/qemu_domain_address.c >
b/src/qemu/qemu_domain_address.c index e28119e4b..3eaa22e39 100644 > ---
a/src/qemu/qemu_domain_address.c +++ > b/src/qemu/qemu_domain_address.c
@@ -532,6 +532,7 @@ >
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >
case VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2: /* xen only */ case >
VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE: case >
VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST: + default: return > 0; } > > @@
-553,6 +554,7 @@ >
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, >
return pciFlags; > > case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: +
default: > return 0; } > > > This causes many of the QEMU test cases to
fail now with a message > > "internal error: Cannot automatically add a
new PCI bus for a device > with connect flags 00" > > > I could
theoretically make the "default:" case return "pciFlags" > when model ==
-1, but this seems dubious because it feels like it is > blindly
assuming that any unknown controller model is PCI based, but > it would
be functionally equivalent to the current behaviour where > USB
accidentally falls through to the IDE code. > > Thoughts ?

 By the time a domain definition operation is completed, the USB model
has been changed from the default of -1 to something explicit, so it
doesn't stay as -1. But I guess the model is being set *after* the PCI
addresses are determined rather than before, which is actually a
potential problem. If someone adds

    <controller type='usb'/>

to their domain, and the default USB controller for that domain is a
PCIe device (or isn't PCI at all) then it will be attached to the wrong
model of PCI controller.

The same applies to the SCSI controller model.

I looked this up for USB, and it gets set (unconditionally) in
virDomainUSBAddressControllerModelToPorts(). Here's the call stack:

  virDomainUSBAddressControllerModelToPorts()
  virDomainUSBAddressSetAddController()
  virDomainUSBAddressSetAddControllers()
  qemuDomainAssignUSBAddresses()

Note that qemuDomainAssignUSBAddresses() is called from
qemuDomainAssignAddresses() just *after* it calls
qemuDomainAssignPCIAddresses(). I'm *guessing* that switching the order
of these two calls will fix this one problem (and hopefully not create
another!)

But we still should be handling the case of -1 in all these switch
statements where the "enum" (which is actually a typecast int) can be
-1. We could try modifying all such variables to use 0 as "unset"
(that's what we do in a lot of cases; it seems like it shouldn't matter
unless the enum values have to correspond to specific integer values
*and* 0 is one of the valid values). Or alternately, we could add a
".....UNSET = -1" to the enum declaration, and make appropriately
modified versions of ENUM_DECL() and ENUM_IMPL() and all their minions.
Maybe there's some simpler solution (we can't just add "default:" to the
switches, since that defeats the entire purpose of typecasting to the
enum so that all enum values must be explicitly given in the switch cases).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to