[libvirt] [PATCH] Revert "maint: prefer enum over int for virstoragefile structs"
Roman Bogorodskiy
bogorodskiy at gmail.com
Sat May 17 07:05:27 UTC 2014
Eric Blake wrote:
> This partially reverts commits b279e52f7 and ea18f8b2.
>
> It turns out our code base is full of:
>
> if ((struct.member = virBlahFromString(str)) < 0)
> goto error;
>
> Meanwhile, the C standard says it is up to the compiler whether
> an enum is signed or unsigned when all of its declared values
> happen to be positive. In my testing (Fedora 20, gcc 4.8.2),
> the compiler picked signed, and nothing changed. But others
> testing with gcc 4.7 got compiler warnings, because it picked
> the enum to be unsigned, but no unsigned value is less than 0.
> Even worse:
This problem happens with clang as well (at least with clang 3.4).
> if ((struct.member = virBlahFromString(str)) <= 0)
> goto error;
>
> is silently compiled without warning, but incorrectly treats -1
> from a bad parse as a large positive number with no warning; and
> without the compiler's help to find these instances, it is a
> nightmare to maintain correctly. We could force signed enums
> a dummy negative declaration in each enum, or cast the result of
> virBlahFromString back to int after assigning to an enum value,
> but that's uglier than what we were trying to cure by directly
> using enum types for struct values. It's better off to just
> live with int members, and use 'switch ((virFoo) struct.member)'
> where we want the compiler to help, than to track down all the
> conversions from string to enum and ensure they don't suffer
> from type problems.
>
> * src/util/virstorageencryption.h: Revert back to int declarations
> with comment about enum usage.
> * src/util/virstoragefile.h: Likewise.
> * src/conf/domain_conf.c: Restore back to casts in switches.
> * src/qemu/qemu_driver.c: Likewise.
> * src/qemu/qemu_command.c: Add cast rather than revert.
> ---
> src/conf/domain_conf.c | 4 ++--
> src/qemu/qemu_command.c | 2 +-
> src/qemu/qemu_driver.c | 8 ++++----
> src/util/virstorageencryption.h | 4 ++--
> src/util/virstoragefile.h | 14 +++++++-------
> 5 files changed, 16 insertions(+), 16 deletions(-)
This patch fixes the problem for me and the approach looks good,
as other workarounds look quite ugly indeed.
Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140517/8b269470/attachment-0001.sig>
More information about the libvir-list
mailing list