[PATCH v4 03/11] qemu: snapshot: Introduce helpers for creating overlays on <transient/> disks

[Ján Tomko]

On a Thursday in 2020, Peter Krempa wrote:
>To implement <transient/> disks we'll need to install an overlay on top
>of the original disk image which will be discarded after the VM is
>turned off. This was initially implemented by qemu but libvirt never
>picked up this option. With blockdev the qemu feature became unsupported
>so we need to do this via the snapshot code anyways.
>
>The helpers introduced in this patch prepare a fake snapshot disk
>definition for a disk which is configured as <transient/> and use it to
>create a snapshot (without actually modifying metada or persistent def).
>
>Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>---
> src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_snapshot.h |  5 +++
> 2 files changed, 95 insertions(+), 1 deletion(-)
>
>diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
>index ca051071aa..d10e7b6b3a 100644
>--- a/src/qemu/qemu_snapshot.c
>+++ b/src/qemu/qemu_snapshot.c
>@@ -986,6 +986,7 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>                            qemuSnapshotDiskDataPtr dd,
>                            virHashTablePtr blockNamedNodeData,
>                            bool reuse,
>+                           bool updateConfig,
>                            qemuDomainAsyncJob asyncJob,
>                            virJSONValuePtr actions)
> {
>@@ -1008,7 +1009,8 @@ qemuSnapshotDiskPrepareOne(virDomainObjPtr vm,
>         return -1;
>
>     /* modify disk in persistent definition only when the source is the same */
>-    if (vm->newDef &&
>+    if (updateConfig &&
>+        vm->newDef &&
>         (persistdisk = virDomainDiskByTarget(vm->newDef, dd->disk->dst)) &&
>         virStorageSourceIsSameLocation(dd->disk->src, persistdisk->src)) {
>
>@@ -1116,6 +1118,54 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObjPtr vm,
>                                        snapctxt->dd + snapctxt->ndd++,
>                                        blockNamedNodeData,
>                                        reuse,
>+                                       true,
>+                                       asyncJob,
>+                                       snapctxt->actions) < 0)
>+            return NULL;
>+    }
>+
>+    return g_steal_pointer(&snapctxt);
>+}
>+
>+
>+static qemuSnapshotDiskContextPtr
>+qemuSnapshotDiskPrepareDisksTransient(virDomainObjPtr vm,
>+                                      virQEMUDriverConfigPtr cfg,
>+                                      virHashTablePtr blockNamedNodeData,
>+                                      qemuDomainAsyncJob asyncJob)
>+{
>+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>+    size_t i;
>+
>+    snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob);
>+
>+    for (i = 0; i < vm->def->ndisks; i++) {
>+        virDomainDiskDefPtr domdisk = vm->def->disks[i];
>+        g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1);
>+
>+        if (!domdisk->transient)
>+            continue;
>+
>+        /* validation code makes sure that we do this only for local disks
>+         * with a file source */
>+        snapdisk->name = g_strdup(domdisk->dst);
>+        snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>+        snapdisk->src = virStorageSourceNew();
>+        snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
>+        snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
>+        snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path);

I'd prefer:
a) an empty line between these assignments and the if()
b) a lowercase name, but I guess standing out is important here as the
file is not meant to stay.

>+        if (virFileExists(snapdisk->src->path)) {
>+            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>+                           _("Overlay file '%s' for transient disk '%s' already exists"),
>+                           snapdisk->src->path, domdisk->dst);
>+            return NULL;
>+        }
>+
>+        if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk,
>+                                       snapctxt->dd + snapctxt->ndd++,
>+                                       blockNamedNodeData,
>+                                       false,
>+                                       false,
>                                        asyncJob,
>                                        snapctxt->actions) < 0)
>             return NULL;
>@@ -1253,6 +1303,45 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObjPtr vm,
> }
>
>
>+/**
>+ * qemuSnapshotCreateDisksTransient:
>+ * @vm: domain object
>+ * @asyncJob: qemu async job type
>+ *
>+ * Creates overlays on top of disks which are configured as <transient/>. Note
>+ * that the validation code ensures that <transient> disks have appropriate
>+ * configuration.
>+ */
>+int
>+qemuSnapshotCreateDisksTransient(virDomainObjPtr vm,
>+                                 qemuDomainAsyncJob asyncJob)
>+{
>+    qemuDomainObjPrivatePtr priv = vm->privateData;
>+    virQEMUDriverPtr driver = priv->driver;
>+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>+    g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL;
>+    g_autoptr(virHashTable) blockNamedNodeData = NULL;
>+
>+    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
>+        if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
>+            return -1;
>+
>+        if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, cfg,
>+                                                               blockNamedNodeData,
>+                                                               asyncJob)))
>+            return -1;
>+
>+        if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0)
>+            return -1;
>+    }
>+
>+    /* the overlays are established, so they can be deleted on a shutdown */

*on shutdown
sounds better to me

>+    priv->inhibitDiskTransientDelete = false;
>+
>+    return 0;
>+}
>+
>+
> static int
> qemuSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,

With the empty line added:
Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200925/475d4a39/attachment-0001.sig>