[libvirt] [libvirt-glib] Add gvir_storage_vol_resize()
From: "Zeeshan Ali (Khattak)" <zeeshanak@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, + GError **err) +{ + GVirStoragePoolInfo* pool_info = NULL; + GVirStorageVolInfo* volume_info = NULL; + gboolean ret = FALSE; + + pool_info = gvir_storage_pool_get_info (vol->priv->pool, err); + if (err != NULL && *err != NULL) + goto cleanup; + volume_info = gvir_storage_vol_get_info (vol, err); + if (err != NULL && *err != NULL) + goto cleanup; + + if (virStorageVolResize(vol->priv->handle, capacity, flags) < 0) { + gvir_set_error_literal(err, + GVIR_STORAGE_VOL_ERROR, + 0, + "Unable to resize volume storage"); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (pool_info) + g_boxed_free(GVIR_TYPE_STORAGE_POOL_INFO, pool_info); + if (volume_info) + gvir_storage_vol_info_free(volume_info); + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.h b/libvirt-gobject/libvirt-gobject-storage-vol.h index 25f683a..459a2ac 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.h +++ b/libvirt-gobject/libvirt-gobject-storage-vol.h @@ -23,6 +23,7 @@ #if !defined(__LIBVIRT_GOBJECT_H__) && !defined(LIBVIRT_GOBJECT_BUILD) #error "Only <libvirt-gobject/libvirt-gobject.h> can be included directly." #endif +#include <libvirt/libvirt.h> #ifndef __LIBVIRT_GOBJECT_STORAGE_VOL_H__ #define __LIBVIRT_GOBJECT_STORAGE_VOL_H__ @@ -65,6 +66,21 @@ typedef enum { GVIR_STORAGE_VOL_STATE_DIR = 2, /* Directory-passthrough based volume */ } GVirStorageVolType; +/** + * GVirStorageVolResizeFlags: + * @GVIR_STORAGE_VOL_RESIZE_NONE: No flags + * @GVIR_STORAGE_VOL_RESIZE_ALLOCATE: force allocation of new size + * @GVIR_STORAGE_VOL_RESIZE_DELTA: size is relative to current + * @GVIR_STORAGE_VOL_RESIZE_SHRINK: allow decrease in capacity. This combined + * with #GVIR_STORAGE_VOL_RESIZE_DELTA, implies a negative delta. + */ +typedef enum { + GVIR_STORAGE_VOL_RESIZE_NONE = 0, + GVIR_STORAGE_VOL_RESIZE_ALLOCATE = VIR_STORAGE_VOL_RESIZE_ALLOCATE, + GVIR_STORAGE_VOL_RESIZE_DELTA = VIR_STORAGE_VOL_RESIZE_DELTA, + GVIR_STORAGE_VOL_RESIZE_SHRINK = VIR_STORAGE_VOL_RESIZE_SHRINK, +} GVirStorageVolResizeFlags; + typedef struct _GVirStorageVolInfo GVirStorageVolInfo; struct _GVirStorageVolInfo { @@ -89,6 +105,10 @@ GVirConfigStorageVol *gvir_storage_vol_get_config(GVirStorageVol *vol, GError **err); GVirStorageVolInfo *gvir_storage_vol_get_info(GVirStorageVol *vol, GError **err); +gboolean gvir_storage_vol_resize(GVirStorageVol *vol, + guint64 capacity, + GVirStorageVolResizeFlags flags, + GError **err); G_END_DECLS diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym index a4f03f7..468bf65 100644 --- a/libvirt-gobject/libvirt-gobject.sym +++ b/libvirt-gobject/libvirt-gobject.sym @@ -126,6 +126,7 @@ LIBVIRT_GOBJECT_0.0.4 { gvir_storage_vol_get_config; gvir_storage_vol_get_info; gvir_storage_vol_delete; + gvir_storage_vol_resize; gvir_connection_handle_get_type; -- 1.7.7.6
On Wed, Feb 01, 2012 at 02:27:28AM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@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 :)
+ GError **err) +{ + GVirStoragePoolInfo* pool_info = NULL; + GVirStorageVolInfo* volume_info = NULL; + gboolean ret = FALSE; + + pool_info = gvir_storage_pool_get_info (vol->priv->pool, err); + if (err != NULL && *err != NULL) + goto cleanup; + volume_info = gvir_storage_vol_get_info (vol, err); + if (err != NULL && *err != NULL) + goto cleanup;
What are pool_info and volume_info used for? Rest of the patch is straightforward, so ACK once libvirt 0.9.10 (11?) is released. Don't forget to update the libvirt requirement in configure.ac if you're the first one to commit something depending on this new release. Thanks, Christophe
+ + if (virStorageVolResize(vol->priv->handle, capacity, flags) < 0) { + gvir_set_error_literal(err, + GVIR_STORAGE_VOL_ERROR, + 0, + "Unable to resize volume storage"); + goto cleanup; + } + + ret = TRUE; + +cleanup: + if (pool_info) + g_boxed_free(GVIR_TYPE_STORAGE_POOL_INFO, pool_info); + if (volume_info) + gvir_storage_vol_info_free(volume_info); + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.h b/libvirt-gobject/libvirt-gobject-storage-vol.h index 25f683a..459a2ac 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.h +++ b/libvirt-gobject/libvirt-gobject-storage-vol.h @@ -23,6 +23,7 @@ #if !defined(__LIBVIRT_GOBJECT_H__) && !defined(LIBVIRT_GOBJECT_BUILD) #error "Only <libvirt-gobject/libvirt-gobject.h> can be included directly." #endif +#include <libvirt/libvirt.h>
#ifndef __LIBVIRT_GOBJECT_STORAGE_VOL_H__ #define __LIBVIRT_GOBJECT_STORAGE_VOL_H__ @@ -65,6 +66,21 @@ typedef enum { GVIR_STORAGE_VOL_STATE_DIR = 2, /* Directory-passthrough based volume */ } GVirStorageVolType;
+/** + * GVirStorageVolResizeFlags: + * @GVIR_STORAGE_VOL_RESIZE_NONE: No flags + * @GVIR_STORAGE_VOL_RESIZE_ALLOCATE: force allocation of new size + * @GVIR_STORAGE_VOL_RESIZE_DELTA: size is relative to current + * @GVIR_STORAGE_VOL_RESIZE_SHRINK: allow decrease in capacity. This combined + * with #GVIR_STORAGE_VOL_RESIZE_DELTA, implies a negative delta. + */ +typedef enum { + GVIR_STORAGE_VOL_RESIZE_NONE = 0, + GVIR_STORAGE_VOL_RESIZE_ALLOCATE = VIR_STORAGE_VOL_RESIZE_ALLOCATE, + GVIR_STORAGE_VOL_RESIZE_DELTA = VIR_STORAGE_VOL_RESIZE_DELTA, + GVIR_STORAGE_VOL_RESIZE_SHRINK = VIR_STORAGE_VOL_RESIZE_SHRINK, +} GVirStorageVolResizeFlags; + typedef struct _GVirStorageVolInfo GVirStorageVolInfo; struct _GVirStorageVolInfo { @@ -89,6 +105,10 @@ GVirConfigStorageVol *gvir_storage_vol_get_config(GVirStorageVol *vol, GError **err); GVirStorageVolInfo *gvir_storage_vol_get_info(GVirStorageVol *vol, GError **err); +gboolean gvir_storage_vol_resize(GVirStorageVol *vol, + guint64 capacity, + GVirStorageVolResizeFlags flags, + GError **err);
G_END_DECLS
diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym index a4f03f7..468bf65 100644 --- a/libvirt-gobject/libvirt-gobject.sym +++ b/libvirt-gobject/libvirt-gobject.sym @@ -126,6 +126,7 @@ LIBVIRT_GOBJECT_0.0.4 { gvir_storage_vol_get_config; gvir_storage_vol_get_info; gvir_storage_vol_delete; + gvir_storage_vol_resize;
gvir_connection_handle_get_type;
-- 1.7.7.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 :)
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. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Feb 1, 2012 at 2:02 PM, Daniel P. Berrange <berrange@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@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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Feb 1, 2012 at 1:03 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Wed, Feb 01, 2012 at 02:27:28AM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
+ GError **err) +{ + GVirStoragePoolInfo* pool_info = NULL; + GVirStorageVolInfo* volume_info = NULL; + gboolean ret = FALSE; + + pool_info = gvir_storage_pool_get_info (vol->priv->pool, err); + if (err != NULL && *err != NULL) + goto cleanup; + volume_info = gvir_storage_vol_get_info (vol, err); + if (err != NULL && *err != NULL) + goto cleanup;
What are pool_info and volume_info used for?
Oh, I forgot to remove those. I also forgot to correct the return value docs.
Rest of the patch is straightforward, so ACK once libvirt 0.9.10 (11?) is released. Don't forget to update the libvirt requirement in configure.ac if you're the first one to commit something depending on this new release.
Oh yes, i forgot that too. :) -- Regards, Zeeshan Ali (Khattak) FSF member#5124
participants (4)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake -
Zeeshan Ali (Khattak)