[libvirt PATCH v2 15/24] qemu_snapshot: add merge to external snapshot delete prepare data

Pavel Hrdina posted 24 patches 2 years, 5 months ago
There is a newer version of this series
[libvirt PATCH v2 15/24] qemu_snapshot: add merge to external snapshot delete prepare data
Posted by Pavel Hrdina 2 years, 5 months ago
Before external snapshot revert every delete operation did block commit
in order to delete a snapshot. But now when user reverts to non-leaf
snapshot deleting leaf snapshot will not have any overlay files so we
can just simply delete the snapshot images.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_snapshot.c | 93 ++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 08cff2a9a2..9c4d26bad5 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2569,9 +2569,26 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
 }
 
 
+/**
+ * qemuSnapshotDeleteExternalPrepareData:
+ * @vm: domain object
+ * @snap: snapshot object
+ * @merge: whether we are just deleting image or not
+ * @externalData: prepared data to delete external snapshot
+ *
+ * Validate if we can delete selected snapshot @snap and prepare all necessary
+ * data that will be used when deleting snapshot as @externalData.
+ *
+ * If @merge is set to true we will merge the deleted snapshot into parent one
+ * instead of just deleting it. This is necessary when operating on snapshot
+ * that has existing overlay files.
+ *
+ * Returns -1 on error, 0 on success.
+ */
 static int
 qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
                                       virDomainMomentObj *snap,
+                                      bool merge,
                                       GSList **externalData)
 {
     ssize_t i;
@@ -2579,7 +2596,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
     g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL;
 
     for (i = 0; i < snapdef->ndisks; i++) {
-        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
         virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
 
         if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
@@ -2591,14 +2607,18 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
                            snapDisk->name);
             return -1;
         }
+    }
+
+    for (i = 0; i < snapdef->ndisks; i++) {
+        virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
+        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
+
+        if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+            continue;
 
         data = g_new0(qemuSnapshotDeleteExternalData, 1);
         data->snapDisk = snapDisk;
 
-        data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name);
-        if (!data->domDisk)
-            return -1;
-
         data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom,
                                                     data->snapDisk->name);
         if (!data->parentDomDisk) {
@@ -2608,39 +2628,46 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
             return -1;
         }
 
-        if (virDomainObjIsActive(vm)) {
-            data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src,
-                                                                data->snapDisk->src,
-                                                                &data->prevDiskSrc);
-            if (!data->diskSrc)
+        if (merge) {
+            data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name);
+            if (!data->domDisk)
                 return -1;
 
-            if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) {
-                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                               _("VM disk source and snapshot disk source are not the same"));
-                return -1;
-            }
+            if (virDomainObjIsActive(vm)) {
+                data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src,
+                                                                    data->snapDisk->src,
+                                                                    &data->prevDiskSrc);
+                if (!data->diskSrc)
+                    return -1;
 
-            data->parentDiskSrc = data->diskSrc->backingStore;
-            if (!virStorageSourceIsBacking(data->parentDiskSrc)) {
-                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                               _("failed to find parent disk source in backing chain"));
-                return -1;
-            }
+                if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) {
+                    virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                                   _("VM disk source and snapshot disk source are not the same"));
+                    return -1;
+                }
 
-            if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) {
-                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                               _("snapshot VM disk source and parent disk source are not the same"));
-                return -1;
+                data->parentDiskSrc = data->diskSrc->backingStore;
+                if (!virStorageSourceIsBacking(data->parentDiskSrc)) {
+                    virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                                   _("failed to find parent disk source in backing chain"));
+                    return -1;
+                }
+
+                if (!virStorageSourceIsSameLocation(data->parentDiskSrc,
+                                                    data->parentDomDisk->src)) {
+                    virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                                   _("snapshot VM disk source and parent disk source are not the same"));
+                    return -1;
+                }
             }
-        }
 
-        data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk);
+            data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk);
 
-        if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) {
-            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                           _("deleting external snapshot that has internal snapshot as parent not supported"));
-            return -1;
+            if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) {
+                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                               _("deleting external snapshot that has internal snapshot as parent not supported"));
+                return -1;
+            }
         }
 
         ret = g_slist_prepend(ret, g_steal_pointer(&data));
@@ -2672,7 +2699,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
     }
 
     /* this also serves as validation whether the snapshot can be deleted */
-    if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0)
+    if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0)
         return -1;
 
     if (!virDomainObjIsActive(vm)) {
@@ -2687,7 +2714,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm,
 
         /* Call the prepare again as some data require that the VM is
          * running to get everything we need. */
-        if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0)
+        if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0)
             return -1;
     } else {
         qemuDomainJobPrivate *jobPriv = vm->job->privateData;
-- 
2.41.0
Re: [libvirt PATCH v2 15/24] qemu_snapshot: add merge to external snapshot delete prepare data
Posted by Peter Krempa 2 years, 4 months ago
On Tue, Jun 27, 2023 at 17:07:18 +0200, Pavel Hrdina wrote:
> Before external snapshot revert every delete operation did block commit
> in order to delete a snapshot. But now when user reverts to non-leaf
> snapshot deleting leaf snapshot will not have any overlay files so we
> can just simply delete the snapshot images.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 93 ++++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 33 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 08cff2a9a2..9c4d26bad5 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2569,9 +2569,26 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
>  }
>  
>  
> +/**
> + * qemuSnapshotDeleteExternalPrepareData:
> + * @vm: domain object
> + * @snap: snapshot object
> + * @merge: whether we are just deleting image or not
> + * @externalData: prepared data to delete external snapshot
> + *
> + * Validate if we can delete selected snapshot @snap and prepare all necessary
> + * data that will be used when deleting snapshot as @externalData.
> + *
> + * If @merge is set to true we will merge the deleted snapshot into parent one
> + * instead of just deleting it. This is necessary when operating on snapshot
> + * that has existing overlay files.
> + *
> + * Returns -1 on error, 0 on success.
> + */
>  static int
>  qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>                                        virDomainMomentObj *snap,
> +                                      bool merge,
>                                        GSList **externalData)
>  {
>      ssize_t i;
> @@ -2579,7 +2596,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>      g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL;
>  
>      for (i = 0; i < snapdef->ndisks; i++) {
> -        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
>          virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
>  
>          if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> @@ -2591,14 +2607,18 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>                             snapDisk->name);
>              return -1;
>          }
> +    }
> +
> +    for (i = 0; i < snapdef->ndisks; i++) {

I didn't really find a reason why you've added another loop here. The
only thing the function does is to prepare data, so I don't think there
is anything that'd require another pass through the disks.

> +        virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> +        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
> +
> +        if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +            continue;

With the extra loop removed:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>