[libvirt] [PATCH v3 18/18] qemu: Implement bulk snapshot operations

Eric Blake posted 18 patches 6 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH v3 18/18] qemu: Implement bulk snapshot operations
Posted by Eric Blake 6 years, 11 months ago
Implement the new flags for bulk snapshot dump and redefine. This
borrows from ideas in the test driver, but is further complicated
by the fact that qemu must write snapshot XML to disk, and thus must
do additional validation after the initial parse to ensure the user
didn't attempt to rename a snapshot with "../" or similar.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/qemu/qemu_domain.c | 26 +++++++++++++----
 src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6f8e03ba36..68298ad4e8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7717,6 +7717,7 @@ qemuDomainDefCopy(virQEMUDriverPtr driver,

 static int
 qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
+                               virDomainObjPtr vm,
                                virDomainDefPtr def,
                                virCPUDefPtr origCPU,
                                unsigned int flags,
@@ -7726,8 +7727,10 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
     virDomainDefPtr copy = NULL;
     virQEMUCapsPtr qemuCaps = NULL;
     virDomainDefFormatData data = { 0 };
+    bool snapshots = flags & VIR_DOMAIN_XML_SNAPSHOTS;

-    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, -1);
+    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
+                  VIR_DOMAIN_XML_SNAPSHOTS, -1);

     if (!(data.caps = virQEMUDriverGetCapabilities(driver, false)))
         goto cleanup;
@@ -7894,6 +7897,15 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
     }

  format:
+    if (snapshots) {
+        if (!vm) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("snapshots XML requested but not provided"));
+            goto cleanup;
+        }
+        data.snapshots = vm->snapshots;
+        data.current_snapshot = vm->current_snapshot;
+    }
     ret = virDomainDefFormatInternal(buf, def, &data,
                                      virDomainDefFormatConvertXMLFlags(flags),
                                      driver->xmlopt);
@@ -7912,19 +7924,21 @@ qemuDomainDefFormatBuf(virQEMUDriverPtr driver,
                        unsigned int flags,
                        virBufferPtr buf)
 {
-    return qemuDomainDefFormatBufInternal(driver, def, NULL, flags, buf);
+    return qemuDomainDefFormatBufInternal(driver, NULL, def, NULL, flags, buf);
 }


 static char *
 qemuDomainDefFormatXMLInternal(virQEMUDriverPtr driver,
+                               virDomainObjPtr vm,
                                virDomainDefPtr def,
                                virCPUDefPtr origCPU,
                                unsigned int flags)
 {
     virBuffer buf = VIR_BUFFER_INITIALIZER;

-    if (qemuDomainDefFormatBufInternal(driver, def, origCPU, flags, &buf) < 0)
+    if (qemuDomainDefFormatBufInternal(driver, vm, def, origCPU, flags,
+                                       &buf) < 0)
         return NULL;

     return virBufferContentAndReset(&buf);
@@ -7936,7 +7950,7 @@ qemuDomainDefFormatXML(virQEMUDriverPtr driver,
                        virDomainDefPtr def,
                        unsigned int flags)
 {
-    return qemuDomainDefFormatXMLInternal(driver, def, NULL, flags);
+    return qemuDomainDefFormatXMLInternal(driver, NULL, def, NULL, flags);
 }


@@ -7955,7 +7969,7 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver,
         origCPU = priv->origCPU;
     }

-    return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
+    return qemuDomainDefFormatXMLInternal(driver, vm, def, origCPU, flags);
 }

 char *
@@ -7972,7 +7986,7 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver,
     if (compatible)
         flags |= VIR_DOMAIN_XML_MIGRATABLE;

-    return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags);
+    return qemuDomainDefFormatXMLInternal(driver, NULL, def, origCPU, flags);
 }


diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 39cc45537d..6a8f8e2bbe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7341,8 +7341,8 @@ static char
     virDomainObjPtr vm;
     char *ret = NULL;

-    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU,
-                  NULL);
+    virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU |
+                  VIR_DOMAIN_XML_SNAPSHOTS, NULL);

     if (!(vm = qemuDomObjFromDomain(dom)))
         goto cleanup;
@@ -15736,6 +15736,33 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def,
 }


+/* Struct and hash-iterator callback used when bulk redefining snapshots */
+struct qemuDomainSnapshotBulk {
+    virDomainObjPtr vm;
+    virQEMUDriverPtr driver;
+    const char *snapshotDir;
+    unsigned int flags;
+};
+
+static int
+qemuDomainSnapshotBulkRedefine(void *payload,
+                               const void *name ATTRIBUTE_UNUSED,
+                               void *opaque)
+{
+    virDomainSnapshotObjPtr snap = payload;
+    struct qemuDomainSnapshotBulk *data = opaque;
+
+    if (qemuDomainSnapshotValidate(snap->def, snap->def->state,
+                                   data->flags) < 0)
+        return -1;
+    if (qemuDomainSnapshotWriteMetadata(data->vm, snap, data->driver->caps,
+                                        data->driver->xmlopt,
+                                        data->snapshotDir) < 0)
+        return -1;
+    return 0;
+}
+
+
 static virDomainSnapshotPtr
 qemuDomainSnapshotCreateXML(virDomainPtr domain,
                             const char *xmlDesc,
@@ -15765,7 +15792,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
                   VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
                   VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
                   VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
-                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
+                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
+                  VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);

     VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
                          VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
@@ -15797,6 +15825,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
         goto cleanup;
     }

+    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
+        struct qemuDomainSnapshotBulk bulk = {
+            .vm = vm,
+            .driver = driver,
+            .snapshotDir = cfg->snapshotDir,
+            .flags = flags,
+        };
+
+        if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid,
+                                          vm->snapshots, &vm->current_snapshot,
+                                          caps, driver->xmlopt,
+                                          parse_flags) < 0)
+            goto cleanup;
+        /* Validate and save the snapshots to disk. Since we don't get
+         * here unless there were no snapshots beforehand, just delete
+         * everything if anything failed, ignoring further errors. */
+        if (virDomainSnapshotForEach(vm->snapshots,
+                                     qemuDomainSnapshotBulkRedefine,
+                                     &bulk) < 0) {
+            virErrorPtr orig_err = virSaveLastError();
+
+            qemuDomainSnapshotDiscardAllMetadata(driver, vm);
+            virSetError(orig_err);
+            virFreeError(orig_err);
+            goto cleanup;
+        }
+        /* Return is arbitrary, so use the first root */
+        snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
+        snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);
+        goto cleanup;
+    }
+
     if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("cannot halt after transient domain snapshot"));
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/18] qemu: Implement bulk snapshot operations
Posted by John Ferlan 6 years, 11 months ago

On 3/4/19 10:34 PM, Eric Blake wrote:
> Implement the new flags for bulk snapshot dump and redefine. This
> borrows from ideas in the test driver, but is further complicated
> by the fact that qemu must write snapshot XML to disk, and thus must
> do additional validation after the initial parse to ensure the user
> didn't attempt to rename a snapshot with "../" or similar.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 26 +++++++++++++----
>  src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 9 deletions(-)
> 

[...]


> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 39cc45537d..6a8f8e2bbe 100644

[...]

>  static virDomainSnapshotPtr
>  qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                              const char *xmlDesc,
> @@ -15765,7 +15792,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                    VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
>                    VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
>                    VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
> -                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
> +                  VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
> +                  VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST, NULL);
> 
>      VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
>                           VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
> @@ -15797,6 +15825,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>          goto cleanup;
>      }
> 
> +    if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST) {
> +        struct qemuDomainSnapshotBulk bulk = {
> +            .vm = vm,
> +            .driver = driver,
> +            .snapshotDir = cfg->snapshotDir,
> +            .flags = flags,
> +        };
> +
> +        if (virDomainSnapshotObjListParse(xmlDesc, vm->def->uuid,
> +                                          vm->snapshots, &vm->current_snapshot,
> +                                          caps, driver->xmlopt,
> +                                          parse_flags) < 0)
> +            goto cleanup;
> +        /* Validate and save the snapshots to disk. Since we don't get
> +         * here unless there were no snapshots beforehand, just delete
> +         * everything if anything failed, ignoring further errors. */
> +        if (virDomainSnapshotForEach(vm->snapshots,
> +                                     qemuDomainSnapshotBulkRedefine,
> +                                     &bulk) < 0) {
> +            virErrorPtr orig_err = virSaveLastError();

I've seen newer code using virErrorPreserveLast

> +
> +            qemuDomainSnapshotDiscardAllMetadata(driver, vm);
> +            virSetError(orig_err);
> +            virFreeError(orig_err);

Similar newer code using virErrorRestore instead of both

> +            goto cleanup;
> +        }
> +        /* Return is arbitrary, so use the first root */
> +        snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
> +        snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);
> +        goto cleanup;
> +    }
> +
>      if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
>          virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                         _("cannot halt after transient domain snapshot"));
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

FYI: I ran the series through Coverity too with no new errors

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