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

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;

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

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.