Re: [libvirt] Default USB controller model vs PCI address assignment

On 02/13/2018 01:43 PM, Daniel P. Berrangé wrote: 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 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. 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 :-/ 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).