[libvirt] [PATCH 4/4] maint: prefer enum over int for virstoragefile structs

[Jim Fehlig]

Eric Blake wrote:
> On 05/14/2014 04:45 PM, Eric Blake wrote:
>   
>> For internal structs, we might as well be type-safe and let the
>> compiler help us with less typing required on our part (getting
>> rid of casts is always nice).  In trying to use enums directly,
>> I noticed two problems in virstoragefile.h that can't be fixed
>> without more invasive refactoring: virStorageSource.format is
>> used as more of a union of multiple enums in storage volume
>> code (so it has to remain an int), and virStorageSourcePoolDef
>> refers to pooltype whose enum is declared in src/conf, but where
>> src/util can't pull in headers from src/conf.
>>     
>
> I'm probably going to have to revert this patch :(
>
> There is a subtle bug here, where the set of machines where the bug
> surfaces depends on the whims of the compiler.
>
>   CC       conf/libvirt_conf_la-domain_conf.lo
> conf/domain_conf.c: In function 'virDomainDiskSourceParse':
> conf/domain_conf.c:4992:9: error: comparison of unsigned expression < 0
> is always false [-Werror=type-limits]
>
> Basically, compilers are allowed to choose whether an enum value with no
> negative constants behaves as signed or unsigned; if the compiler
> chooses unsigned, but we then do 'if ((value = virBlahFromString(str)) <
> 0)', we cause the compiler warning; if the compiler chooses signed, then
> everything just works.  Worse, if we do 'if ((value =
> virBlahFromString(str)) <= 0)', the compiler silently does the wrong
> thing (-1 returned from virBlahFromString is promoted to unsigned, so
> the condition is false, and we are stuck using an out-of-range value
> without warning).
>
> On IRC, I talked about several workarounds that could keep us with an
> enum type; each looks worse than what this change would cure, which is
> why I think reverting is the only sane option.
>
> 1. Force the comparison in int (yucky because it adds a cast, but my
> stated intention of this commit was to avoid casts, and requires a code
> audit):
>
> if ((int)(value = virBlahFromString(str)) <= 0)
>
> 2. Use temporaries (no ugly casts, but requires a code audit):
>
> int tmp;
> if ((tmp = virBlahFromString(str)) < 0)
> ...
> value = tmp;
>   

I have a patch along these lines to fix the build failures, but agree
that it does not sit well with your intentions of this patch.

> 3. Force the enum to be signed (yucky because we have to inject an
> otherwise unused -1 element in each enum):
>
> typedef enum {
>   VIR_BLAH__FORCE_SIGNED = -1,
>   VIR_BLAH_A,
>   VIR_BLAH_B,
>   VIR_BLAH_LAST
> } virBlah;
>
> 4. Enhance VIR_ENUM_DECL/VIR_ENUM_IMPL to provide yet another conversion
> function.  The existing virBlahFromString(str) returns -1 on failure or
> enum values on success; the new virBlahEnumFromString(str) returns -1 on
> failure or 0 on success with the correct value stored into the address.
> (yucky because you now have to know which of the two FromString variants
> to call based on what type you are assigning into, because 'int*' and
> 'enum*' are not compatible types):
>
> enum value;
> int other;
> if (virBlahEnumFromString(str, &value) < 0)
> ...
> if ((other = virBlahFromString(str)) < 0)
> ,,,
>
> 5. Alter VIR_ENUM_DECL/VIR_ENUM_IMPL to make the FromString variant have
> new semantics - the _LAST value is now hard-coded as the failure value
> rather than -1, and the return signature is an enum (ugly because it
> requires a full tree audit, and because checking for ==_LAST is not as
> nice as checking for < 0; and it gets worse when also excluding 0)
>
> if ((value = virBlahFromString(str)) == VIR_BLAH_LAST)
>    ...error about unknown value
>
> value = virBlahFromString(str);
> if (!value || value == VIR_BLAH_LAST)
>    ...error about bad value
>   

Heh, hard to argue that any of these options _improve_ the code base.

> Any other opinions, or should I just go ahead with the revert?  This is
> breaking the build for some people, depending on their gcc version.
>   

I'm torn.  This intent of the patch is a nice improvement, but with
diminishing returns when adjusting it to work with some versions of gcc
and clang.

Regards,
Jim