Re: [libvirt] [libvirt-glib] Add gvir_storage_vol_resize()
On Wed, Feb 1, 2012 at 2:02 PM, Daniel P. Berrange <berrange(a)redhat.com> wrote:
On Wed, Feb 01, 2012 at 12:03:43PM +0100, Christophe Fergeau wrote:
> On Wed, Feb 01, 2012 at 02:27:28AM +0200, Zeeshan Ali (Khattak) wrote:
> > From: "Zeeshan Ali (Khattak)" <zeeshanak(a)gnome.org>
> >
> > Add wrapper for virStorageVolResize().
> > ---
> > libvirt-gobject/libvirt-gobject-storage-vol.c | 45 +++++++++++++++++++++++++
> > libvirt-gobject/libvirt-gobject-storage-vol.h | 20 +++++++++++
> > libvirt-gobject/libvirt-gobject.sym | 1 +
> > 3 files changed, 66 insertions(+), 0 deletions(-)
> >
> > diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c
b/libvirt-gobject/libvirt-gobject-storage-vol.c
> > index 491e2e6..f6f0c50 100644
> > --- a/libvirt-gobject/libvirt-gobject-storage-vol.c
> > +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c
> > @@ -299,3 +299,48 @@ gboolean gvir_storage_vol_delete(GVirStorageVol *vol,
> >
> > return TRUE;
> > }
> > +
> > +/**
> > + * gvir_storage_vol_resize:
> > + * @vol: the storage volume to resize
> > + * @capacity: the new capacity of the volume
> > + * @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.
OTOH, not specifying the exact type increases the chances of developer
passing wrong values.
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.
On C-level this will only mean a slightly non-intuitive API (as you
can't see from the signature what the hell is supposed to be passed to
flags) but in high-level languages it might even mean that developer
have to cast the enums to guint64 explicitly (which would be annoying
as the reason for this won't be obvious to most).
Anyway, I am strong against what you guys are suggesting as I prefer
intuitive API over caring about such corner cases but in the end its
up to you (Daniel) so just say 'nay' in reply and I'll change it to
guint64.
--
Regards,
Zeeshan Ali (Khattak)
FSF member#5124