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@xxxxxxxxxx> --- src/qemu/qemu_snapshot.c | 141 ++++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 222afe62e1..f560cf270c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/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, - virDomainMomentObj *snap, - const char *op, - bool try_all, - int ndisks) +qemuSnapshotForEachQcow2One(virStorageSource *src, + const char *op, + const char *snapname) +{ + 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++) { - g_autoptr(virCommand) cmd = virCommandNewArgList("qemu-img", "snapshot", - op, snap->def->name, NULL); - int format = virDomainDiskGetFormat(def->disks[i]); + /* pre-checks */ + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDef *disk = 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 (try_all) { - /* Continue on even in the face of error, since other - * 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); - } + if (create && + disk->src->format > VIR_STORAGE_FILE_NONE && + disk->src->format != VIR_STORAGE_FILE_QCOW2) { virReportError(VIR_ERR_OPERATION_INVALID, _("Disk device '%1$s' does not support snapshotting"), - def->disks[i]->dst); + disk->dst); 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; - virCommandAddArg(cmd, virDomainDiskGetSource(def->disks[i])); + 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 (virCommandRun(cmd, NULL) < 0) { - if (try_all) { - VIR_WARN("skipping snapshot action on %s", - def->disks[i]->dst); + 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; - continue; - } else if (STREQ(op, "-c") && i) { - /* We must roll back partial creation by deleting - * all earlier snapshots. */ - qemuSnapshotForEachQcow2Internal(def, snap, "-d", false, i); + virResetLastError(); } - return -1; } } 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 - * failure, 1 if we skipped a disk due to try_all. */ -static int -qemuSnapshotForEachQcow2(virDomainDef *def, - virDomainMomentObj *snap, - const char *op, - bool try_all) -{ - return qemuSnapshotForEachQcow2Internal(def, snap, op, try_all, def->ndisks); + if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK || + snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL || + (disk->src->format > VIR_STORAGE_FILE_NONE && + disk->src->format != VIR_STORAGE_FILE_QCOW2)) + continue; + + ignore_value(qemuSnapshotForEachQcow2One(disk->src, "-d", snap->def->name)); + } + + 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 { -- 2.47.0