[libvirt PATCH 12/20] qemu_snapshot: add blockCommit to external snapshot delete prepare data

Pavel Hrdina posted 20 patches 1 year, 4 months ago
There is a newer version of this series
[libvirt PATCH 12/20] qemu_snapshot: add blockCommit to external snapshot delete prepare data
Posted by Pavel Hrdina 1 year, 4 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 | 102 ++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 40 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index d545c984ce..6ff18d7a9e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
 static int
 qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
                                       virDomainMomentObj *snap,
+                                      bool blockCommit,
                                       GSList **externalData)
 {
     ssize_t i;
@@ -2483,7 +2484,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)
@@ -2495,56 +2495,78 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
                            snapDisk->name);
             return -1;
         }
+    }
+
+    if (!blockCommit) {
+        if (!snap->parent) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("snapshot '%s' doesn't have a parent snapshot"),
+                             snapdef->parent.name);
+            return -1;
+        }
+
+        snapdef = virDomainSnapshotObjGetDef(snap->parent);
+    }
+
+    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) {
-            virReportError(VIR_ERR_OPERATION_FAILED,
-                           _("failed to find disk '%s' in snapshot VM XML"),
-                           snapDisk->name);
-            return -1;
-        }
-
-        if (virDomainObjIsActive(vm)) {
-            data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src,
-                                                                data->snapDisk->src,
-                                                                &data->prevDiskSrc);
-            if (!data->diskSrc)
+        if (blockCommit) {
+            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"));
+            data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom,
+                                                        data->snapDisk->name);
+            if (!data->parentDomDisk) {
+                virReportError(VIR_ERR_OPERATION_FAILED,
+                               _("failed to find disk '%s' in snapshot VM XML"),
+                               snapDisk->name);
                 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 (virDomainObjIsActive(vm)) {
+                data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src,
+                                                                    data->snapDisk->src,
+                                                                    &data->prevDiskSrc);
+                if (!data->diskSrc)
+                    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;
+                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;
+                }
+
+                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));
@@ -2576,7 +2598,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)) {
@@ -2591,7 +2613,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.39.2
Re: [libvirt PATCH 12/20] qemu_snapshot: add blockCommit to external snapshot delete prepare data
Posted by Peter Krempa 1 year, 3 months ago
On Mon, Mar 13, 2023 at 16:42:13 +0100, 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 | 102 ++++++++++++++++++++++++---------------
>  1 file changed, 62 insertions(+), 40 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index d545c984ce..6ff18d7a9e 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
>  static int
>  qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>                                        virDomainMomentObj *snap,
> +                                      bool blockCommit,
>                                        GSList **externalData)

This function is starting to get into the region of needing
documentation after the flag was added. Also I'd call it a bit
differently to de-emphasise that block commit is used, but rather that
we are merging images.

Either way:

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