[libvirt PATCH v2 23/24] qemu_snapshot: add checks for external snapshot deletion

Pavel Hrdina posted 24 patches 2 years, 5 months ago
There is a newer version of this series
[libvirt PATCH v2 23/24] qemu_snapshot: add checks for external snapshot deletion
Posted by Pavel Hrdina 2 years, 5 months ago
With the introduction of external snapshot revert support we need to
error out in some cases when trying to delete some snapshots.

If users reverts to non-leaf snapshots and would try to delete it after
the revert is done it would not work currently as this operation would
require using block-stream which is not implemented for now as in this
case the snapshot has two children so the disk files have multiple
overlays.

Similarly if user reverts to non-leaf snapshot and would try to delete
snapshot that is non-leaf but not in currently active snapshot chain we
would still need to use block-commit operation. The issue here is that
in order to do that we would have to start new qemu process with
different domain definition than what is currently used by the domain.
If the current domain would be running it would complicate things even
more so this operation is not yet supported.

If user creates new snapshot after reverting to non-leaf snapshot it
creates a new branch. Deleting snapshot with multiple children will
require block-stream which is not implemented for now.

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 9246c02f12..9e8a7f2f9f 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3636,6 +3636,8 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
     }
 
     if (virDomainSnapshotIsExternal(snap)) {
+        virDomainMomentObj *current = virDomainSnapshotGetCurrent(vm->snapshots);
+
         if (qemuDomainHasBlockjob(vm, false)) {
             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                            _("cannot delete external snapshots when there is another active block job"));
@@ -3647,6 +3649,25 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
                            _("deletion of external disk snapshots with children not supported"));
             return -1;
         }
+
+        if (snap == current && snap->nchildren != 0) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("deletion of active external snapshot that is not a leaf snapshot is not supported"));
+            return -1;
+        }
+
+        if (snap != current && snap->nchildren != 0 &&
+            virDomainMomentIsAncestor(snap, current)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("deletion of non-leaf external snapshot that is not in active chain is not supported"));
+            return -1;
+        }
+
+        if (snap->nchildren > 1) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("deletion of external disk snapshot with multiple children snapshots not supported"));
+            return -1;
+        }
     }
 
     return 0;
-- 
2.41.0
Re: [libvirt PATCH v2 23/24] qemu_snapshot: add checks for external snapshot deletion
Posted by Peter Krempa 2 years, 4 months ago
On Tue, Jun 27, 2023 at 17:07:26 +0200, Pavel Hrdina wrote:
> With the introduction of external snapshot revert support we need to
> error out in some cases when trying to delete some snapshots.
> 
> If users reverts to non-leaf snapshots and would try to delete it after
> the revert is done it would not work currently as this operation would
> require using block-stream which is not implemented for now as in this
> case the snapshot has two children so the disk files have multiple
> overlays.
> 
> Similarly if user reverts to non-leaf snapshot and would try to delete
> snapshot that is non-leaf but not in currently active snapshot chain we
> would still need to use block-commit operation. The issue here is that
> in order to do that we would have to start new qemu process with
> different domain definition than what is currently used by the domain.
> If the current domain would be running it would complicate things even
> more so this operation is not yet supported.
> 
> If user creates new snapshot after reverting to non-leaf snapshot it
> creates a new branch. Deleting snapshot with multiple children will
> require block-stream which is not implemented for now.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++

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