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
Attachment:
pgpOQDbIzgbGW.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list