On 02/01/2012 12:44 PM, Zeeshan Ali (Khattak) wrote: >>>> + * @flags: the flags >>>> + * @err: Return location for errors, or NULL >>>> + * >>>> + * Changes the capacity of the storage volume @vol to @capacity. >>>> + * >>>> + * Returns: the new capacity of the volume on success, 0 otherwise >>>> + */ >>>> +gboolean gvir_storage_vol_resize(GVirStorageVol *vol, >>>> + guint64 capacity, >>>> + GVirStorageVolResizeFlags flags, >>> >>> We're probably already doing that in other places, I may have already >>> mentioned it, but I'm a bit uncomfortable with using the enum type for >>> bitfields. If I'm not mistaken, if we use GVirStorageVolResizeFlags, >>> passing a value which is not one of the enum values (such as DELTA | >>> SHRINK) is undefined behaviour in the C spec. I'm fine with us deciding >>> it's not important, but it's still worth mentioning :) > > Seriously, do we have to care about every corner-case imaginable? Why > would anyone pass anything other than the enums supported when the > type of the parameter is clearly indicated in function signature? Even > if for some weird reason, somebody somehow manages to do this, AFAIK > every modern (enough) compiler will give him/her warning about this. > And if compiler isn't going to, libvirt is erroring out if you pass > anything other than support flags. The problem is that enum virStorageVolResizeFlags in libvirt.h is _not_ a strict enum where no other values are valid, but a definition of bit values that are designed to be combined together. That is, the flags value of 6 (aka VIR_STORAGE_VOL_RESIZE_DELTA | VIR_STROAGE_VOL_RESIZE_SHRINK) is _not_ part of the C enum, but _is_ a valid value to pass into the flags field of the C API. We are insisting on using integer, rather than enum types, for the flags parameter, precisely because we want to only list N flags rather than all 2**N valid combinations of the flags in our enum. Since the bitwise OR of enums is valid in C, but produces an int value, rather than maintaining the type of the enum, it is easier to encourage proper bitwise-OR by giving the flags parameter the type that results from use of bitwise-OR. > > OTOH, not specifying the exact type increases the chances of developer > passing wrong values. Perhaps, but that's why every libvirt API also has a check for all bits falling within the expected range, and erroring out if any unknown bits are found (at least that were unknown at the time the .so was compiled). That way, if a future .so understands more bits, and you don't know whether you are talking to a newer or older server, you can try the bit out, and be guaranteed of a sane failure if you are talking to the older server, rather than getting in a potentially undefined state where the server ignored your bit and you think the server is in a different state than it really is because you assume the bit was acted on. > >> Yeah, for methods which are 'flags', we should use guint64 as the >> type. Only use the enum types for things which really are enums, >> not bitfields. guint64 may be a bit big, since libvirt.h only uses 32-bit flags; but I agree with Dan's complaint that we should not be using the flag enum type as the signature, at least not for any flags parameter designed to be a bitwise-OR of defined bit values. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list