Re: [libvirt] [PATCHv3 1/3] qemu: hostdev: Refactor PCI passhrough handling

>> + switch ((virDomainHostdevSubsysPciBackendType) >> + dev->source.subsys.u.pci.backend) { > > I'm assuming you're doing the typecast so that you're forced to have a > case for every enum value. Do we have an official policy on that? I see > similar typecasts for switch statements in a few places, but I don't > know that I'm so fond of it... I started adding this on multiple places earlier. I don't know if there's a official strategy, but I tend to prefer it as I was bitten a few times by adding a new enum value and missed a few switches where I actually should handle those. That's why I'm adding the typecasts where possible.

> > (Personally, I think if we're going to do enforce explicit listing of > all cases in switch statements for an attribute that has an enum > associated with it, it would be better to define the field with the > actual enum type rather than int - these are internal data structures > that are never passed from one machine to another, so it's not like > there would ever be any compatibility problem.) That would be a ideal global solution. Unfortunately as the SOMETypeFromString macroed functions return -1 on failure almost all variables holding the value got by this functions are declared as int rather than the appropriate enum value. To avoid the need for a separate intermediate variable we tend to declare the holding vars as int.

>> + break; >> + >> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT: >> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM: >> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST: > > Again - I dislike having these sentinel values showing up in switch > statements (searching the source I see it a lot), as it could lead > someone to believe that those are actually valid values. I may be > outvoted on this though :-) I think that the benefits of the compiler warning in case you add a new variable on places you might have forgotten outweigh a few occurences of unused type in the code.