Extract creation of qcow2 files for external snapshots to separate
function as we will need it for external snapshot revert code.

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index bbef753db6..06ed20c815 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -181,35 +181,21 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef,
 }
 
 
-/* The domain is expected to be locked and inactive. */
 static int
-qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver,
-                                   virDomainObj *vm,
-                                   virDomainMomentObj *snap)
-{
-    return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false);
-}
-
-
-/* The domain is expected to be locked and inactive. */
-static int
-qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
-                                   virDomainObj *vm,
-                                   virDomainMomentObj *snap,
-                                   bool reuse)
+qemuSnapshotCreateQcow2Files(virDomainObj *vm,
+                             virDomainSnapshotDef *snapdef,
+                             virBitmap *created,
+                             bool reuse)
 {
     size_t i;
-    virDomainSnapshotDiskDef *snapdisk;
-    virDomainDiskDef *defdisk;
     const char *qemuImgPath;
-    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-    int ret = -1;
     g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-    virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
-    g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks);
+    virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver;
+    virDomainSnapshotDiskDef *snapdisk = NULL;
+    virDomainDiskDef *defdisk = NULL;
 
     if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
-        goto cleanup;
+        return -1;
 
     /* If reuse is true, then qemuSnapshotPrepare already
      * ensured that the new files exist, and it was up to the user to
@@ -218,6 +204,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
         g_autoptr(virCommand) cmd = NULL;
         snapdisk = &(snapdef->disks[i]);
         defdisk = vm->def->disks[i];
+
         if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
             continue;
 
@@ -225,7 +212,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
             snapdisk->src->format = VIR_STORAGE_FILE_QCOW2;
 
         if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0)
-            goto cleanup;
+            return -1;
 
         /* creates cmd line args: qemu-img create -f qcow2 -o */
         if (!(cmd = virCommandNewArgList(qemuImgPath,
@@ -234,7 +221,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
                                          virStorageFileFormatTypeToString(snapdisk->src->format),
                                          "-o",
                                          NULL)))
-            goto cleanup;
+            return -1;
 
         /* adds cmd line arg: backing_fmt=format,backing_file=/path/to/backing/file */
         virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=",
@@ -251,9 +238,39 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
             ignore_value(virBitmapSetBit(created, i));
 
         if (virCommandRun(cmd, NULL) < 0)
-            goto cleanup;
+            return -1;
     }
 
+    return 0;
+}
+
+
+/* The domain is expected to be locked and inactive. */
+static int
+qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver,
+                                   virDomainObj *vm,
+                                   virDomainMomentObj *snap)
+{
+    return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false);
+}
+
+
+/* The domain is expected to be locked and inactive. */
+static int
+qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
+                                   virDomainObj *vm,
+                                   virDomainMomentObj *snap,
+                                   bool reuse)
+{
+    virDomainSnapshotDiskDef *snapdisk;
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    int ret = -1;
+    virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
+    g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks);
+
+    if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0)
+        goto cleanup;
+
     /* update disk definitions */
     if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0)
         goto cleanup;
-- 
2.39.2

On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote:
> Extract creation of qcow2 files for external snapshots to separate
> function as we will need it for external snapshot revert code.

Hmm, I don't think I like where this is going. I presume you want to use
this code to create the new overlay images.

If you want to use this also in cases where the VM was live you might
run into scenarios where the qemu-img code will not be able to handle
the overlay creation, specifically on networked storage.

I think we'll need to create the overlay images as part of the startup
of the reverted VM via QMP exactly as we are creating overlays currently
for snapshots.

Even when today's API will not allow reversion of non-local snapshots (I
didn't check further in this series yet) I don't think we should go the
way of using qemu-img at all.

On Tue, Apr 04, 2023 at 01:09:41PM +0200, Peter Krempa wrote:
> On Mon, Mar 13, 2023 at 16:42:08 +0100, Pavel Hrdina wrote:
> > Extract creation of qcow2 files for external snapshots to separate
> > function as we will need it for external snapshot revert code.
> 
> Hmm, I don't think I like where this is going. I presume you want to use
> this code to create the new overlay images.

Correct

> If you want to use this also in cases where the VM was live you might
> run into scenarios where the qemu-img code will not be able to handle
> the overlay creation, specifically on networked storage.

We always kill the qemu process when reverting so I figured that
qemu-img should be good enough to create the new overlay because the
qemu process is not running at that point. Did not think we allow
snapshot creation for qcow2 images on networked storage as it would not
work for offline VM where we also use this qemu-img approach.

Looking at the code for running VM we are using QMP to create the
overlay files and I did not manage to find any check to limit it only
for local images. If that's the case we have IMHO really bad
inconsistent behavior where we would allow creating new snapshots for
non-local images if the VM is running but fail to do so if the VM is
offline.

> I think we'll need to create the overlay images as part of the startup
> of the reverted VM via QMP exactly as we are creating overlays currently
> for snapshots.
> 
> Even when today's API will not allow reversion of non-local snapshots (I
> didn't check further in this series yet) I don't think we should go the
> way of using qemu-img at all.

There is no check for that as I assumed (probably incorrectly) that we
allow creating snapshot only for local images. If that's not the case
then yes we need to use the QMP commands to create overlays.

Pavel