[libvirt] [libvirt-glib] gobject: Also delete storage volume from its pool's list
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> Without this patch, storage pool still lists the volume even after it is deleted. Related Boxes bug: https://bugzilla.gnome.org/show_bug.cgi?id=688724 --- libvirt-gobject/libvirt-gobject-storage-pool.c | 23 +++++++++++++++++++++++ libvirt-gobject/libvirt-gobject-storage-vol.c | 4 ++++ 2 files changed, 27 insertions(+) diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..df7e77c 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h" #define GVIR_STORAGE_POOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_POOL, GVirStoragePoolPrivate)) @@ -1129,3 +1130,25 @@ gboolean gvir_storage_pool_delete_finish(GVirStoragePool *pool, return TRUE; } + +G_GNUC_INTERNAL gboolean gvir_storage_pool_delete_vol(GVirStoragePool *pool, + GVirStorageVol *volume) +{ + GVirStoragePoolPrivate *priv; + gboolean ret = FALSE; + + g_return_val_if_fail(GVIR_IS_STORAGE_POOL(pool), FALSE); + g_return_val_if_fail(GVIR_IS_STORAGE_VOL(volume), FALSE); + + priv = pool->priv; + g_mutex_lock(priv->lock); + if (priv->volumes != NULL) { + const gchar *name = gvir_storage_vol_get_name(volume); + ret = g_hash_table_remove(priv->volumes, name); + } else { + g_warn_if_reached(); + } + g_mutex_unlock(priv->lock); + + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index c7ebb45..4352d30 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h" #define GVIR_STORAGE_VOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_VOL, GVirStorageVolPrivate)) @@ -309,6 +310,9 @@ gboolean gvir_storage_vol_delete(GVirStorageVol *vol, g_return_val_if_fail(GVIR_IS_STORAGE_VOL(vol), FALSE); g_return_val_if_fail(err == NULL || *err == NULL, FALSE); + if (!gvir_storage_pool_delete_vol(vol->priv->pool, vol)) + return FALSE; + if (virStorageVolDelete(vol->priv->handle, flags) < 0) { gvir_set_error_literal(err, GVIR_STORAGE_VOL_ERROR, -- 1.8.0
On 20.11.2012 19:51, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Without this patch, storage pool still lists the volume even after it is deleted.
Related Boxes bug: https://bugzilla.gnome.org/show_bug.cgi?id=688724 --- libvirt-gobject/libvirt-gobject-storage-pool.c | 23 +++++++++++++++++++++++ libvirt-gobject/libvirt-gobject-storage-vol.c | 4 ++++ 2 files changed, 27 insertions(+)
diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..df7e77c 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
#define GVIR_STORAGE_POOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_POOL, GVirStoragePoolPrivate)) @@ -1129,3 +1130,25 @@ gboolean gvir_storage_pool_delete_finish(GVirStoragePool *pool,
return TRUE; } + +G_GNUC_INTERNAL gboolean gvir_storage_pool_delete_vol(GVirStoragePool *pool, + GVirStorageVol *volume) +{ + GVirStoragePoolPrivate *priv; + gboolean ret = FALSE; + + g_return_val_if_fail(GVIR_IS_STORAGE_POOL(pool), FALSE); + g_return_val_if_fail(GVIR_IS_STORAGE_VOL(volume), FALSE);
The latter one has been already checked as can be seen in context of the last chunk. The first one should not fail since it is us who inserted the value into volume->priv->vol; But assuming this function may be used somewhere else in the future these checks are actually correct - maybe my assumptions won't last then. Moreover, it doesn't hurt to check when playing around, right?
+ + priv = pool->priv; + g_mutex_lock(priv->lock); + if (priv->volumes != NULL) { + const gchar *name = gvir_storage_vol_get_name(volume); + ret = g_hash_table_remove(priv->volumes, name); + } else { + g_warn_if_reached(); + } + g_mutex_unlock(priv->lock); + + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index c7ebb45..4352d30 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
#define GVIR_STORAGE_VOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_VOL, GVirStorageVolPrivate)) @@ -309,6 +310,9 @@ gboolean gvir_storage_vol_delete(GVirStorageVol *vol, g_return_val_if_fail(GVIR_IS_STORAGE_VOL(vol), FALSE); g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
+ if (!gvir_storage_pool_delete_vol(vol->priv->pool, vol)) + return FALSE; + if (virStorageVolDelete(vol->priv->handle, flags) < 0) { gvir_set_error_literal(err, GVIR_STORAGE_VOL_ERROR,
ACK Michal
On Wed, Nov 21, 2012 at 09:30:02AM +0100, Michal Privoznik wrote:
On 20.11.2012 19:51, Zeeshan Ali (Khattak) wrote: The latter one has been already checked as can be seen in context of the last chunk. The first one should not fail since it is us who inserted the value into volume->priv->vol;
Imo it's good to check that things are consistant with whatever assumptions have to be true, if the hash gets corrupted somehow, we'll at least get a warning that things are not in order. Most arguments to public/semi-public APIs are sanity-checked this way.
But assuming this function may be used somewhere else in the future these checks are actually correct - maybe my assumptions won't last then. Moreover, it doesn't hurt to check when playing around, right?
Well, it hurts in the sense that it has a runtime cost, which can be non-negligible when these checks are run in a tight loop, but I don't think these specific functions will be called that often. Christophe
On Tue, Nov 20, 2012 at 08:51:23PM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Without this patch, storage pool still lists the volume even after it is deleted.
Related Boxes bug: https://bugzilla.gnome.org/show_bug.cgi?id=688724 --- libvirt-gobject/libvirt-gobject-storage-pool.c | 23 +++++++++++++++++++++++ libvirt-gobject/libvirt-gobject-storage-vol.c | 4 ++++ 2 files changed, 27 insertions(+)
diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..df7e77c 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
libvirt-gobject-storage-pool-private.h is missing from this diff
#define GVIR_STORAGE_POOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_POOL, GVirStoragePoolPrivate)) @@ -1129,3 +1130,25 @@ gboolean gvir_storage_pool_delete_finish(GVirStoragePool *pool,
return TRUE; } + +G_GNUC_INTERNAL gboolean gvir_storage_pool_delete_vol(GVirStoragePool *pool, + GVirStorageVol *volume) +{ + GVirStoragePoolPrivate *priv; + gboolean ret = FALSE; + + g_return_val_if_fail(GVIR_IS_STORAGE_POOL(pool), FALSE); + g_return_val_if_fail(GVIR_IS_STORAGE_VOL(volume), FALSE); + + priv = pool->priv; + g_mutex_lock(priv->lock); + if (priv->volumes != NULL) { + const gchar *name = gvir_storage_vol_get_name(volume); + ret = g_hash_table_remove(priv->volumes, name); + } else { + g_warn_if_reached(); + } + g_mutex_unlock(priv->lock); + + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index c7ebb45..4352d30 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
#define GVIR_STORAGE_VOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_VOL, GVirStorageVolPrivate)) @@ -309,6 +310,9 @@ gboolean gvir_storage_vol_delete(GVirStorageVol *vol, g_return_val_if_fail(GVIR_IS_STORAGE_VOL(vol), FALSE); g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
+ if (!gvir_storage_pool_delete_vol(vol->priv->pool, vol)) + return FALSE; +
I'm not sure this should be fatal. Even if we report an error when this occurs, we probably should try to call virStorageVolDelete nonetheless. You need to fill 'err' if you return FALSE here. Rest of the patch looks good. Christophe
On Wed, Nov 21, 2012 at 12:49 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, Nov 20, 2012 at 08:51:23PM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Without this patch, storage pool still lists the volume even after it is deleted.
Related Boxes bug: https://bugzilla.gnome.org/show_bug.cgi?id=688724 --- libvirt-gobject/libvirt-gobject-storage-pool.c | 23 +++++++++++++++++++++++ libvirt-gobject/libvirt-gobject-storage-vol.c | 4 ++++ 2 files changed, 27 insertions(+)
diff --git a/libvirt-gobject/libvirt-gobject-storage-pool.c b/libvirt-gobject/libvirt-gobject-storage-pool.c index 8f579a1..df7e77c 100644 --- a/libvirt-gobject/libvirt-gobject-storage-pool.c +++ b/libvirt-gobject/libvirt-gobject-storage-pool.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
libvirt-gobject-storage-pool-private.h is missing from this diff
#define GVIR_STORAGE_POOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_POOL, GVirStoragePoolPrivate)) @@ -1129,3 +1130,25 @@ gboolean gvir_storage_pool_delete_finish(GVirStoragePool *pool,
return TRUE; } + +G_GNUC_INTERNAL gboolean gvir_storage_pool_delete_vol(GVirStoragePool *pool, + GVirStorageVol *volume) +{ + GVirStoragePoolPrivate *priv; + gboolean ret = FALSE; + + g_return_val_if_fail(GVIR_IS_STORAGE_POOL(pool), FALSE); + g_return_val_if_fail(GVIR_IS_STORAGE_VOL(volume), FALSE); + + priv = pool->priv; + g_mutex_lock(priv->lock); + if (priv->volumes != NULL) { + const gchar *name = gvir_storage_vol_get_name(volume); + ret = g_hash_table_remove(priv->volumes, name); + } else { + g_warn_if_reached(); + } + g_mutex_unlock(priv->lock); + + return ret; +} diff --git a/libvirt-gobject/libvirt-gobject-storage-vol.c b/libvirt-gobject/libvirt-gobject-storage-vol.c index c7ebb45..4352d30 100644 --- a/libvirt-gobject/libvirt-gobject-storage-vol.c +++ b/libvirt-gobject/libvirt-gobject-storage-vol.c @@ -29,6 +29,7 @@ #include "libvirt-glib/libvirt-glib.h" #include "libvirt-gobject/libvirt-gobject.h" #include "libvirt-gobject-compat.h" +#include "libvirt-gobject-storage-pool-private.h"
#define GVIR_STORAGE_VOL_GET_PRIVATE(obj) \ (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_STORAGE_VOL, GVirStorageVolPrivate)) @@ -309,6 +310,9 @@ gboolean gvir_storage_vol_delete(GVirStorageVol *vol, g_return_val_if_fail(GVIR_IS_STORAGE_VOL(vol), FALSE); g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
+ if (!gvir_storage_pool_delete_vol(vol->priv->pool, vol)) + return FALSE; +
I'm not sure this should be fatal. Even if we report an error when this occurs, we probably should try to call virStorageVolDelete nonetheless. You need to fill 'err' if you return FALSE here.
On second thought gvir_storage_pool_delete_vol() doesn't need to return FALSE at all. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
participants (3)
-
Christophe Fergeau -
Michal Privoznik -
Zeeshan Ali (Khattak)