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

[Eric Blake]

On 10/07/2013 08:16 AM, Peter Krempa wrote:

>>> +    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.

I actually like it as well - any time you can make the compiler catch
your mistakes at compile time, rather than waiting until an obscure
error at runtime, is a win, even if the syntax is a bit awkward (casting
to force the correct type checking, and listing dead case labels to
appease the compiler).


>>
>> (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.

The cast inside the switch is a reasonable compromise, since it is often
easier to pass int around (and in public APIs, we _have_ to pass int
around - not all enums are private-use only).


>>> +        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.

Agreed.  Maybe we should patch HACKING to capture the outcome of this
discussion.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131007/53850934/attachment-0001.sig>