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

[Eric Blake]

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.

-- 
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: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140516/01dbb60d/attachment-0001.sig>