qemuSnapshotForEachQcow2: Refactor

Refactor the function to avoid recursive call to rollback and simplify
calling parameters.

To achieve that most of the fatal checks are extracted into a dedicated
loop that runs before modifying the disk state thus removing the need to
rollback altoghether. Since rollback is still necessary when creation of
the snapshot fails half-way through the rollback is extracted to handle
only that scenario.

Additionally callers would only pass the old 'try_all' argument as true
on all non-creation ("-c") modes. This means that we can infer it from
the operation instead of passing it as an extra argument.

This refactor will also make it much simpler to implement handling of
the NVRAM pflash backing file (in case it's qcow2) for internal
snapshots.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

src/qemu/qemu_snapshot.c

@@ -249,86 +249,121 @@ qemuSnapshotCreateQcow2Files(virDomainDef *def,
 }
 
 
-/* The domain is expected to be locked and inactive. Return -1 on normal
- * failure, 1 if we skipped a disk due to try_all.  */
 static int
-qemuSnapshotForEachQcow2Internal(virDomainDef *def,
+qemuSnapshotForEachQcow2One(virStorageSource *src,
-                                 virDomainMomentObj *snap,
+                            const char *op,
-                                 const char *op,
+                            const char *snapname)
-                                 bool try_all,
+{
-                                 int ndisks)
+    g_autoptr(virCommand) cmd = NULL;
+
+    cmd = virCommandNewArgList("qemu-img", "snapshot",
+                               op, snapname, src->path, NULL);
+
+    if (virCommandRun(cmd, NULL) < 0)
+        return -1;
+
+    return 0;
+}
+
+
+/**
+ * qemuSnapshotForEachQcow2:
+ *
+ * @def: domain definition
+ * @snap: snapshot object
+ * @op: 'qemu-img snapshot' operation flag, one of "-c", "-d", "-a"
+ *
+ * Applies the selected 'qemu-img snapshot' operation @op on all relevant QCOW2
+ * images of @def. In case when @op is "-c" (create) any failure is fatal and
+ * rolled back. Otherwise the selected operation @op is applied on all images
+ * regardless of failure.
+ *
+ * Returns: -1 on errror; 0 on complete success; 1 on partial success in
+ * permissive modes.
+ */
+static int
+qemuSnapshotForEachQcow2(virDomainDef *def,
+                         virDomainMomentObj *snap,
+                         const char *op)
 {
     virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
     size_t i;
     bool skipped = false;
+    bool create = STREQ(op, "-c");
+    size_t nrollback = -1;
+    virErrorPtr orig_err;
 
-    for (i = 0; i < ndisks; i++) {
+    /* pre-checks */
-        g_autoptr(virCommand) cmd = virCommandNewArgList("qemu-img", "snapshot",
+    for (i = 0; i < def->ndisks; i++) {
-                                                         op, snap->def->name, NULL);
+        virDomainDiskDef *disk = def->disks[i];
-        int format = virDomainDiskGetFormat(def->disks[i]);
 
-        /* FIXME: we also need to handle LVM here */
         if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK ||
             snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL)
             continue;
 
-        if (!virStorageSourceIsLocalStorage(def->disks[i]->src)) {
+        if (!virStorageSourceIsLocalStorage(disk->src)) {
             virReportError(VIR_ERR_OPERATION_INVALID,
                            _("can't manipulate inactive snapshots of disk '%1$s'"),
-                           def->disks[i]->dst);
+                           disk->dst);
             return -1;
         }
 
-        if (format > 0 && format != VIR_STORAGE_FILE_QCOW2) {
+        if (create &&
-            if (try_all) {
+            disk->src->format > VIR_STORAGE_FILE_NONE &&
-                /* Continue on even in the face of error, since other
+            disk->src->format != VIR_STORAGE_FILE_QCOW2) {
-                 * disks in this VM may have the same snapshot name.
-                 */
-                VIR_WARN("skipping snapshot action on %s",
-                         def->disks[i]->dst);
-                skipped = true;
-                continue;
-            } else if (STREQ(op, "-c") && i) {
-                /* We must roll back partial creation by deleting
-                 * all earlier snapshots.  */
-                qemuSnapshotForEachQcow2Internal(def, snap, "-d", false, i);
-            }
             virReportError(VIR_ERR_OPERATION_INVALID,
                            _("Disk device '%1$s' does not support snapshotting"),
-                           def->disks[i]->dst);
+                           disk->dst);
-            return -1;
-        }
-
-        virCommandAddArg(cmd, virDomainDiskGetSource(def->disks[i]));
-
-        if (virCommandRun(cmd, NULL) < 0) {
-            if (try_all) {
-                VIR_WARN("skipping snapshot action on %s",
-                         def->disks[i]->dst);
-                skipped = true;
-                continue;
-            } else if (STREQ(op, "-c") && i) {
-                /* We must roll back partial creation by deleting
-                 * all earlier snapshots.  */
-                qemuSnapshotForEachQcow2Internal(def, snap, "-d", false, i);
-            }
             return -1;
         }
     }
 
+    for (i = 0; i < def->ndisks; i++) {
+        virDomainDiskDef *disk = def->disks[i];
+
+        if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK ||
+            snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL)
+            continue;
+
+        if (disk->src->format > VIR_STORAGE_FILE_NONE &&
+            disk->src->format != VIR_STORAGE_FILE_QCOW2) {
+            VIR_WARN("skipping 'qemu-img snapshot %s' action on non-qcow2 disk '%s'",
+                     op, disk->dst);
+            skipped = true;
+            continue;
+        }
+
+        if (qemuSnapshotForEachQcow2One(disk->src, op, snap->def->name) < 0) {
+            if (create) {
+                nrollback = i;
+                virErrorPreserveLast(&orig_err);
+                goto rollback;
+            } else {
+                VIR_WARN("failed 'qemu-img snapshot %s' action on '%s'",
+                         op, disk->dst);
+                skipped = true;
+                virResetLastError();
+            }
+        }
+    }
+
     return skipped ? 1 : 0;
-}
 
+ rollback:
+    for (i = 0; i < nrollback; i++) {
+        virDomainDiskDef *disk = def->disks[i];
 
-/* The domain is expected to be locked and inactive. Return -1 on normal
+        if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK ||
- * failure, 1 if we skipped a disk due to try_all.  */
+            snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL ||
-static int
+            (disk->src->format > VIR_STORAGE_FILE_NONE &&
-qemuSnapshotForEachQcow2(virDomainDef *def,
+             disk->src->format != VIR_STORAGE_FILE_QCOW2))
-                         virDomainMomentObj *snap,
+            continue;
-                         const char *op,
+
-                         bool try_all)
+        ignore_value(qemuSnapshotForEachQcow2One(disk->src, "-d", snap->def->name));
-{
+    }
-    return qemuSnapshotForEachQcow2Internal(def, snap, op, try_all, def->ndisks);
+
+    virErrorRestore(&orig_err);
+    return -1;
 }
 
 
@@ -337,7 +372,7 @@ static int
 qemuSnapshotCreateInactiveInternal(virDomainObj *vm,
                                    virDomainMomentObj *snap)
 {
-    return qemuSnapshotForEachQcow2(vm->def, snap, "-c", false);
+    return qemuSnapshotForEachQcow2(vm->def, snap, "-c");
 }
 
 
@@ -2634,7 +2669,7 @@ qemuSnapshotInternalRevertInactive(virDomainObj *vm,
     }
 
     /* Try all disks, but report failure if we skipped any.  */
-    if (qemuSnapshotForEachQcow2(def, snap, "-a", true) != 0)
+    if (qemuSnapshotForEachQcow2(def, snap, "-a") != 0)
         return -1;
 
     return 0;
@@ -4002,7 +4037,7 @@ qemuSnapshotDiscardImpl(virDomainObj *vm,
                 if (qemuSnapshotDiscardExternal(vm, snap, externalData) < 0)
                     return -1;
             } else {
-                if (qemuSnapshotForEachQcow2(def, snap, "-d", true) < 0)
+                if (qemuSnapshotForEachQcow2(def, snap, "-d") < 0)
                     return -1;
             }
         } else {