On 10/15/2015 10:38 AM, Wido den Hollander wrote:
> When a RBD volume has snapshots it can not be removed.
>
> This patch introduces a new flag to force volume removal,
> VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS.
>
> With this flag any existing snapshots will be removed prior to
> removing the volume.
>
> No existing mechanism in libvirt allowed us to pass such information,
> so that's why a new flag was introduced.
>
> Signed-off-by: Wido den Hollander <wido@xxxxxxxxx>
> ---
> include/libvirt/libvirt-storage.h | 1 +
> src/storage/storage_backend_rbd.c | 59 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h
> index 453089e..80da5a3 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -115,6 +115,7 @@ typedef enum {
> typedef enum {
> VIR_STORAGE_VOL_DELETE_NORMAL = 0, /* Delete metadata only (fast) */
> VIR_STORAGE_VOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros (slow) */
> + VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS = 2, /* Force removal of volume, even if in use */
s/2/(1 << 1)
> } virStorageVolDeleteFlags;
>
> typedef enum {
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 5ae4713..71f7d10 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -428,9 +428,13 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
> {
> int ret = -1;
> int r = 0;
> + int max_snaps = 128;
> + int i, snap_count, protected;
fail syntax-check - must use "size_t i;" since it's a loop counter
> virStorageBackendRBDState ptr;
> ptr.cluster = NULL;
> ptr.ioctx = NULL;
> + rbd_snap_info_t *snaps;
> + rbd_image_t image = NULL;
>
> VIR_DEBUG("Removing RBD image %s/%s", pool->def->source.name, vol->name);
>
> @@ -443,6 +447,60 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
> if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0)
> goto cleanup;
>
Is any of this useful without VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS? I
also think you'd leak whatever rbd_snap_list places into the 'snaps'
entries since rbd_snap_list_end would only be called if the bit was set.
Can this be turned into a helper routine instead, e.g.
if (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS) {
if (virStorageBackendRBDCleanupSnapshots(...args...) < 0)
goto cleanup;
}
Your new helper should set some kind of error on failure; otherwise,
you'll get a generic libvirt failed for some reason type error which
could be confusing. IOW: Anywhere you 'goto cleanup;' will need some
sort of message.
I'm assuming 'failure' means the rbd_remove() was going to fail anyway,
so going to cleanup on failure to cleanup is "ok"
> + r = rbd_open(ptr.ioctx, vol->name, &image, NULL);
> + if (r < 0) {
> + virReportSystemError(-r, _("failed to open the RBD image '%s'"),
> + vol->name);
> + goto cleanup;
> + }
> +
> + do {
> + if (VIR_ALLOC_N(snaps, max_snaps))
> + goto cleanup;
> +
> + snap_count = rbd_snap_list(image, snaps, &max_snaps);
> + if (snap_count <= 0) {
> + VIR_FREE(snaps);
> + }
^^ fails syntax-check (the extra { } for one-liner)
> + } while (snap_count == -ERANGE);
Too bad there wasn't a way to pass (image, NULL, &max_snaps); where
max_snaps = 0 to start out with in order to get the number to use for
max_snaps. Although I assume the loop would only need to be executed at
most 2 times. So no big deal.
> +
> + VIR_DEBUG("Found %d snapshots for volume %s/%s", snap_count,
> + pool->def->source.name, vol->name);
> +
> + if (snap_count > 0 && (flags & VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS)) {
> + for (i = 0; i < snap_count; i++) {
> + if (rbd_snap_is_protected(image, snaps[i].name, &protected))
> + goto cleanup;
No error message?
> +
> + if (protected == 1) {
> + VIR_DEBUG("Snapshot %s/%s@%s is protected needs to be "
> + "unprotected", pool->def->source.name, vol->name,
> + snaps[i].name);
> +
> + if (rbd_snap_unprotect(image, snaps[i].name) < 0)
> + goto cleanup;
No error message?
> + }
> +
> + VIR_DEBUG("Removing snapshot %s/%s@%s", pool->def->source.name,
> + vol->name, snaps[i].name);
> +
> + r = rbd_snap_remove(image, snaps[i].name);
> + if (r < 0) {
> + virReportSystemError(-r, _("failed to remove snapshot '%s/%s@%s'"),
> + pool->def->source.name, vol->name,
> + snaps[i].name);
> + goto cleanup;
> + }
> + }
> +
> + rbd_snap_list_end(snaps);
> + }
> +
> + if (rbd_close(image) < 0)
> + goto cleanup;
perhaps this should be moved to cleanup and changed to:
if (image)
rbd_close(image);
because all those goto's inside your "if" condition will not close image.
Also does rbd_snap_list_end need to be called if any of the goto
cleanup's happen? Can I assume that will run through the list of snaps
and free the snaps[i].name? and anything else - so yes, I think it
needs to be in cleanup as well (lots easier when this code is a helper).
John
> +
> + VIR_DEBUG("Removing volume %s/%s", pool->def->source.name, vol->name);
> +
> r = rbd_remove(ptr.ioctx, vol->name);
> if (r < 0 && (-r) != ENOENT) {
> virReportSystemError(-r, _("failed to remove volume '%s/%s'"),
> @@ -453,6 +511,7 @@ static int virStorageBackendRBDDeleteVol(virConnectPtr conn,
> ret = 0;
>
> cleanup:
> + VIR_FREE(snaps);
> virStorageBackendRBDCloseRADOSConn(&ptr);
> return ret;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list