qemu: snapshot: Refactors prior to introduction of <transient> disk support

[PATCH 5/6] qemuSnapshotCreateActiveExternalDisks: Extract actual snapshot creation to 'qemuSnapshotDiskCreate'

Posted by Peter Krempa 5 years, 4 months ago

Extract the code which invokes the monitor and finalizes the snapshot
into a separate function for easier reuse.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_snapshot.c | 67 ++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index b9b640058c..6fe925763e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1182,57 +1182,42 @@ qemuSnapshotDiskUpdateSource(virDomainObjPtr vm,
 }


-/* The domain is expected to be locked and active. */
 static int
-qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
-                                      virDomainObjPtr vm,
-                                      virDomainMomentObjPtr snap,
-                                      virHashTablePtr blockNamedNodeData,
-                                      unsigned int flags,
-                                      virQEMUDriverConfigPtr cfg,
-                                      qemuDomainAsyncJob asyncJob)
+qemuSnapshotDiskCreate(qemuSnapshotDiskContextPtr snapctxt,
+                       virQEMUDriverConfigPtr cfg)
 {
-    qemuDomainObjPrivatePtr priv = vm->privateData;
-    int rc;
+    qemuDomainObjPrivatePtr priv = snapctxt->vm->privateData;
+    virQEMUDriverPtr driver = priv->driver;
     size_t i;
-    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
-    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
-
-    if (virDomainObjCheckActive(vm) < 0)
-        return -1;
+    int rc;

-    /* prepare a list of objects to use in the vm definition so that we don't
-     * have to roll back later */
-    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
-                                             blockNamedNodeData, asyncJob)))
-        return -1;

     /* check whether there's anything to do */
     if (snapctxt->ndd == 0)
         return 0;

-    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+    if (qemuDomainObjEnterMonitorAsync(driver, snapctxt->vm, snapctxt->asyncJob) < 0)
         return -1;

     rc = qemuMonitorTransaction(priv->mon, &snapctxt->actions);

-    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+    if (qemuDomainObjExitMonitor(driver, snapctxt->vm) < 0)
         rc = -1;

     for (i = 0; i < snapctxt->ndd; i++) {
         qemuSnapshotDiskDataPtr dd = snapctxt->dd + i;

-        virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
+        virDomainAuditDisk(snapctxt->vm, dd->disk->src, dd->src, "snapshot", rc >= 0);

         if (rc == 0)
-            qemuSnapshotDiskUpdateSource(vm, dd);
+            qemuSnapshotDiskUpdateSource(snapctxt->vm, dd);
     }

     if (rc < 0)
         return -1;

-    if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0 ||
-        (vm->newDef && virDomainDefSave(vm->newDef, driver->xmlopt,
+    if (virDomainObjSave(snapctxt->vm, driver->xmlopt, cfg->stateDir) < 0 ||
+        (snapctxt->vm->newDef && virDomainDefSave(snapctxt->vm->newDef, driver->xmlopt,
                                         cfg->configDir) < 0))
         return -1;

@@ -1240,6 +1225,34 @@ qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
 }


+/* The domain is expected to be locked and active. */
+static int
+qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
+                                      virDomainMomentObjPtr snap,
+                                      virHashTablePtr blockNamedNodeData,
+                                      unsigned int flags,
+                                      virQEMUDriverConfigPtr cfg,
+                                      qemuDomainAsyncJob asyncJob)
+{
+    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        return -1;
+
+    /* prepare a list of objects to use in the vm definition so that we don't
+     * have to roll back later */
+    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
+                                             blockNamedNodeData, asyncJob)))
+        return -1;
+
+    if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
+        return -1;
+
+    return 0;
+}
+
+
 static int
 qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
                                  virDomainObjPtr vm,
@@ -1366,7 +1379,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,

     /* the domain is now paused if a memory snapshot was requested */

-    if ((ret = qemuSnapshotCreateActiveExternalDisks(driver, vm, snap,
+    if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
                                                      blockNamedNodeData, flags, cfg,
                                                      QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
         goto cleanup;
-- 
2.26.2

Re: [PATCH 5/6] qemuSnapshotCreateActiveExternalDisks: Extract actual snapshot creation to 'qemuSnapshotDiskCreate'

Posted by Ján Tomko 5 years, 4 months ago

On a Thursday in 2020, Peter Krempa wrote:
>Extract the code which invokes the monitor and finalizes the snapshot
>into a separate function for easier reuse.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_snapshot.c | 67 ++++++++++++++++++++++++----------------
> 1 file changed, 40 insertions(+), 27 deletions(-)
>
>diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
>index b9b640058c..6fe925763e 100644
>--- a/src/qemu/qemu_snapshot.c
>+++ b/src/qemu/qemu_snapshot.c
>@@ -1182,57 +1182,42 @@ qemuSnapshotDiskUpdateSource(virDomainObjPtr vm,
> }
>
>
>-/* The domain is expected to be locked and active. */
> static int
>-qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
>-                                      virDomainObjPtr vm,
>-                                      virDomainMomentObjPtr snap,
>-                                      virHashTablePtr blockNamedNodeData,
>-                                      unsigned int flags,
>-                                      virQEMUDriverConfigPtr cfg,
>-                                      qemuDomainAsyncJob asyncJob)
>+qemuSnapshotDiskCreate(qemuSnapshotDiskContextPtr snapctxt,
>+                       virQEMUDriverConfigPtr cfg)
> {
>-    qemuDomainObjPrivatePtr priv = vm->privateData;
>-    int rc;
>+    qemuDomainObjPrivatePtr priv = snapctxt->vm->privateData;
>+    virQEMUDriverPtr driver = priv->driver;
>     size_t i;
>-    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
>-    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>-
>-    if (virDomainObjCheckActive(vm) < 0)
>-        return -1;
>+    int rc;
>
>-    /* prepare a list of objects to use in the vm definition so that we don't
>-     * have to roll back later */
>-    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
>-                                             blockNamedNodeData, asyncJob)))
>-        return -1;
>

This leaves a double space here.

Jano

>     /* check whether there's anything to do */
>     if (snapctxt->ndd == 0)
>         return 0;
>
>-    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>+    if (qemuDomainObjEnterMonitorAsync(driver, snapctxt->vm, snapctxt->asyncJob) < 0)
>         return -1;
>
>     rc = qemuMonitorTransaction(priv->mon, &snapctxt->actions);
>
>-    if (qemuDomainObjExitMonitor(driver, vm) < 0)
>+    if (qemuDomainObjExitMonitor(driver, snapctxt->vm) < 0)
>         rc = -1;
>
>     for (i = 0; i < snapctxt->ndd; i++) {
>         qemuSnapshotDiskDataPtr dd = snapctxt->dd + i;
>
>-        virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
>+        virDomainAuditDisk(snapctxt->vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
>
>         if (rc == 0)
>-            qemuSnapshotDiskUpdateSource(vm, dd);
>+            qemuSnapshotDiskUpdateSource(snapctxt->vm, dd);
>     }
>
>     if (rc < 0)
>         return -1;
>
>-    if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0 ||
>-        (vm->newDef && virDomainDefSave(vm->newDef, driver->xmlopt,
>+    if (virDomainObjSave(snapctxt->vm, driver->xmlopt, cfg->stateDir) < 0 ||
>+        (snapctxt->vm->newDef && virDomainDefSave(snapctxt->vm->newDef, driver->xmlopt,
>                                         cfg->configDir) < 0))
>         return -1;
>
>@@ -1240,6 +1225,34 @@ qemuSnapshotCreateActiveExternalDisks(virQEMUDriverPtr driver,
> }
>
>
>+/* The domain is expected to be locked and active. */
>+static int
>+qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
>+                                      virDomainMomentObjPtr snap,
>+                                      virHashTablePtr blockNamedNodeData,
>+                                      unsigned int flags,
>+                                      virQEMUDriverConfigPtr cfg,
>+                                      qemuDomainAsyncJob asyncJob)
>+{
>+    bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
>+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>+
>+    if (virDomainObjCheckActive(vm) < 0)
>+        return -1;
>+
>+    /* prepare a list of objects to use in the vm definition so that we don't
>+     * have to roll back later */
>+    if (!(snapctxt = qemuSnapshotDiskPrepare(vm, snap, cfg, reuse,
>+                                             blockNamedNodeData, asyncJob)))
>+        return -1;
>+
>+    if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
>+        return -1;
>+
>+    return 0;
>+}
>+
>+
> static int
> qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,
>@@ -1366,7 +1379,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>
>     /* the domain is now paused if a memory snapshot was requested */
>
>-    if ((ret = qemuSnapshotCreateActiveExternalDisks(driver, vm, snap,
>+    if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap,
>                                                      blockNamedNodeData, flags, cfg,
>                                                      QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
>         goto cleanup;
>-- 
>2.26.2
>

[PATCH 1/6] qemu: snapshot: Rename 'qemuSnapshotCreateDiskActive' to 'qemuSnapshotCreateActiveExternalDisks'
[PATCH 2/6] qemuSnapshotDiskUpdateSource: Extract 'driver' and 'blockdev' from 'vm'
[PATCH 3/6] qemuSnapshotDiskPrepare/Cleanup: simplify passing of 'driver' and 'blockdev'
[PATCH 4/6] qemu: snapshot: Introduce qemuSnapshotDiskContext
[PATCH 5/6] qemuSnapshotCreateActiveExternalDisks: Extract actual snapshot creation to 'qemuSnapshotDiskCreate'
[PATCH 6/6] qemuSnapshotDiskPrepare: rename to qemuSnapshotDiskPrepareActiveExternal