Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
---
 src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++++++++++++++++----
 src/qemu/qemu_driver.c |  7 ++++---
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ba3fff607a..73c41788f8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
                                              op, try_all, def->ndisks);
 }
 
+static int
+qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
+{
+    int i;
+    virDomainSnapshotDiskDefPtr snap_disk;
+
+    // FIXME: libvirt stores the VM definition and the list of disks that
+    // snapshot has been taken of since 0.9.5. Before that we must assume that
+    // all disks from within the VM definition have been snapshotted and that
+    // they all were internal disks. Did libvirt support external snapshots
+    // before 0.9.5? If so, if we ever end up in this code path we may incur
+    // data loss as we'll delete whole QEMU file thinking it's an external
+    // snapshot.
+    if (!snap->def->dom) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       _("Snapshots created using libvirt 9.5 and containing "
+                         "external disk snapshots cannot be safely "
+                         "discarded."));
+        return -1;
+    }
+
+    for (i = 0; i < snap->def->ndisks; ++i) {
+        snap_disk = &(snap->def->disks[i]);
+        if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+            continue;
+        if (unlink(snap_disk->src->path) < 0)
+            VIR_WARN("Failed to unlink snapshot disk '%s'",
+                     snap_disk->src->path);
+    }
+    return 0;
+}
+
 /* Discard one snapshot (or its metadata), without reparenting any children.  */
 int
 qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
@@ -8260,10 +8292,15 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
 
     if (!metadata_only) {
         if (!virDomainObjIsActive(vm)) {
-            /* Ignore any skipped disks */
-            if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
-                                               true) < 0)
-                goto cleanup;
+            if (virDomainSnapshotIsExternal(snap)) {
+                if (qemuDomainSnapshotDiscardExternal(snap) < 0)
+                    goto cleanup;
+            } else {
+                /* Ignore any skipped disks */
+                if (qemuDomainSnapshotForEachQcow2(driver, vm, snap, "-d",
+                                                   true) < 0)
+                    goto cleanup;
+            }
         } else {
             priv = vm->privateData;
             qemuDomainObjEnterMonitor(driver, vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index eb104d306b..7cd44d6957 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16662,7 +16662,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
     if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
         goto endjob;
 
-    if (!metadata_only) {
+    if (!metadata_only && virDomainObjIsActive(vm)) {
         if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) &&
             virDomainSnapshotIsExternal(snap))
             external++;
@@ -16673,8 +16673,9 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
                                                &external);
         if (external) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("deletion of %d external disk snapshots not "
-                             "supported yet"), external);
+                           _("deletion of %d external disk snapshots while the "
+                             "domain is active is not supported yet"),
+                           external);
             goto endjob;
         }
     }
-- 
2.17.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Sun, Oct 21, 2018 at 19:38:50 +0300, Povilas Kanapickas wrote:
> Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
> ---
>  src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++++++++++++++++----
>  src/qemu/qemu_driver.c |  7 ++++---
>  2 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff607a..73c41788f8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
>                                               op, try_all, def->ndisks);
>  }
>  
> +static int
> +qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
> +{
> +    int i;
> +    virDomainSnapshotDiskDefPtr snap_disk;
> +
> +    // FIXME: libvirt stores the VM definition and the list of disks that
> +    // snapshot has been taken of since 0.9.5. Before that we must assume that
> +    // all disks from within the VM definition have been snapshotted and that
> +    // they all were internal disks. Did libvirt support external snapshots
> +    // before 0.9.5? If so, if we ever end up in this code path we may incur
> +    // data loss as we'll delete whole QEMU file thinking it's an external
> +    // snapshot.

No any external snapshot does have the state, so the condition below is
dead code.

> +    if (!snap->def->dom) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("Snapshots created using libvirt 9.5 and containing "
> +                         "external disk snapshots cannot be safely "
> +                         "discarded."));
> +        return -1;
> +    }
> +
> +    for (i = 0; i < snap->def->ndisks; ++i) {
> +        snap_disk = &(snap->def->disks[i]);
> +        if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +            continue;
> +        if (unlink(snap_disk->src->path) < 0)
> +            VIR_WARN("Failed to unlink snapshot disk '%s'",
> +                     snap_disk->src->path);

So this will ever only work properly for leaf snapshots. If you try to
delete a snapshot which has children you'll destroy any of it's
children. Since they are not deleted here we can't do it without that
since reverting to them would be broken.

Ideally we want to merge the changes to all relevant snapshots so that
the intermediate files and snapshot metadata can be deleted without data
loss even in the middle of the chain/tree.

Also one other note, snapshots are possible on networked storage where
unlink will not work, so this needs to be made more general.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list