The 'virCommand' helpers already look up the full path to the binary in PATH if it's not specified. This means that the qemu driver doesn't have to lookup and store the path to 'qemu-img' in the conf object but rather can be cleaned up to use this new infrastructure. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_conf.h | 3 --- src/qemu/qemu_domain.c | 20 +------------------- src/qemu/qemu_domain.h | 2 -- src/qemu/qemu_driver.c | 3 --- src/qemu/qemu_snapshot.c | 33 +++++++++------------------------ 5 files changed, 10 insertions(+), 51 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 157aba9e18..23a900193e 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -271,9 +271,6 @@ struct _virQEMUDriver { /* Immutable pointer, self-locking APIs */ virDomainObjList *domains; - /* Immutable pointer */ - char *qemuImgBinary; - /* Immutable pointer, lockless APIs. Pointless abstraction */ ebtablesContext *ebtables; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c798ef37fd..e8e72e5091 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5671,17 +5671,6 @@ qemuDomainLogAppendMessage(virQEMUDriver *driver, } -/* Locate an appropriate 'qemu-img' binary. */ -const char * -qemuFindQemuImgBinary(virQEMUDriver *driver) -{ - if (!driver->qemuImgBinary) - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to find qemu-img")); - - return driver->qemuImgBinary; -} - int qemuDomainSnapshotWriteMetadata(virDomainObj *vm, virDomainMomentObj *snapshot, @@ -5727,18 +5716,11 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver, int ndisks) { virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); - const char *qemuimgbin; size_t i; bool skipped = false; - qemuimgbin = qemuFindQemuImgBinary(driver); - if (qemuimgbin == NULL) { - /* qemuFindQemuImgBinary set the error */ - return -1; - } - for (i = 0; i < ndisks; i++) { - g_autoptr(virCommand) cmd = virCommandNewArgList(qemuimgbin, "snapshot", + g_autoptr(virCommand) cmd = virCommandNewArgList("qemu-img", "snapshot", op, snap->def->name, NULL); int format = virDomainDiskGetFormat(def->disks[i]); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1ae421e5f2..091b27823b 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -665,8 +665,6 @@ int qemuDomainLogAppendMessage(virQEMUDriver *driver, const char *fmt, ...) G_GNUC_PRINTF(3, 4); -const char *qemuFindQemuImgBinary(virQEMUDriver *driver); - int qemuDomainSnapshotWriteMetadata(virDomainObj *vm, virDomainMomentObj *snapshot, virDomainXMLOption *xmlopt, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 72a9542c0b..90d55e53ec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -679,8 +679,6 @@ qemuStateInitialize(bool privileged, virPidFileAcquire(cfg->stateDir, "driver", getpid())) < 0) goto error; - qemu_driver->qemuImgBinary = virFindFileInPath("qemu-img"); - if (!(qemu_driver->lockManager = virLockManagerPluginNew(cfg->lockManagerName ? cfg->lockManagerName : "nop", @@ -1065,7 +1063,6 @@ qemuStateCleanup(void) virCPUDefFree(qemu_driver->hostcpu); virObjectUnref(qemu_driver->caps); ebtablesContextFree(qemu_driver->ebtables); - VIR_FREE(qemu_driver->qemuImgBinary); virObjectUnref(qemu_driver->domains); virObjectUnref(qemu_driver->nbdkitCapsCache); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 795522da21..35c8d67d20 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -185,7 +185,6 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, /** * qemuSnapshotCreateQcow2Files: - * @driver: QEMU driver * @def: domain definition * @snapdef: snapshot definition * @created: bitmap to store which disks were created @@ -196,20 +195,15 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, * Returns 0 on success, -1 on error. */ static int -qemuSnapshotCreateQcow2Files(virQEMUDriver *driver, - virDomainDef *def, +qemuSnapshotCreateQcow2Files(virDomainDef *def, virDomainSnapshotDef *snapdef, virBitmap *created) { size_t i; - const char *qemuImgPath; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virDomainSnapshotDiskDef *snapdisk = NULL; virDomainDiskDef *defdisk = NULL; - if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) - return -1; - for (i = 0; i < snapdef->ndisks; i++) { g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); @@ -225,7 +219,7 @@ qemuSnapshotCreateQcow2Files(virQEMUDriver *driver, return -1; /* creates cmd line args: qemu-img create -f qcow2 -o */ - if (!(cmd = virCommandNewArgList(qemuImgPath, + if (!(cmd = virCommandNewArgList("qemu-img", "create", "-f", virStorageFileFormatTypeToString(snapdisk->src->format), @@ -281,7 +275,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, /* If reuse is true, then qemuSnapshotPrepare already * ensured that the new files exist, and it was up to the user to * create them correctly. */ - if (!reuse && qemuSnapshotCreateQcow2Files(driver, vm->def, snapdef, created) < 0) + if (!reuse && qemuSnapshotCreateQcow2Files(vm->def, snapdef, created) < 0) goto cleanup; /* update disk definitions */ @@ -2300,7 +2294,6 @@ qemuSnapshotRevertExternalActive(virDomainObj *vm, /** * qemuSnapshotRevertExternalInactive: - * @vm: domain object * @tmpsnapdef: temporary snapshot definition * @domdef: offline domain definition * @@ -2310,17 +2303,15 @@ qemuSnapshotRevertExternalActive(virDomainObj *vm, * Returns 0 on success, -1 on error. */ static int -qemuSnapshotRevertExternalInactive(virDomainObj *vm, - virDomainSnapshotDef *tmpsnapdef, +qemuSnapshotRevertExternalInactive(virDomainSnapshotDef *tmpsnapdef, virDomainDef *domdef) { - virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; g_autoptr(virBitmap) created = NULL; int ret = -1; created = virBitmapNew(tmpsnapdef->ndisks); - if (qemuSnapshotCreateQcow2Files(driver, domdef, tmpsnapdef, created) < 0) + if (qemuSnapshotCreateQcow2Files(domdef, tmpsnapdef, created) < 0) goto cleanup; if (qemuSnapshotDomainDefUpdateDisk(domdef, tmpsnapdef, false) < 0) @@ -2613,7 +2604,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, return -1; } - if (qemuSnapshotRevertExternalInactive(vm, tmpsnapdef, + if (qemuSnapshotRevertExternalInactive(tmpsnapdef, *inactiveConfig) < 0) { return -1; } @@ -3443,22 +3434,16 @@ qemuSnapshotSetInvalid(virDomainObj *vm, static void -qemuSnapshotUpdateBackingStore(virDomainObj *vm, - qemuSnapshotDeleteExternalData *data) +qemuSnapshotUpdateBackingStore(qemuSnapshotDeleteExternalData *data) { GSList *cur = NULL; - const char *qemuImgPath; - virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; - - if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) - return; for (cur = data->disksWithBacking; cur; cur = g_slist_next(cur)) { struct _qemuSnapshotDisksWithBackingStoreData *backingData = cur->data; g_autoptr(virCommand) cmd = NULL; /* creates cmd line args: qemu-img create -f qcow2 -o */ - if (!(cmd = virCommandNewArgList(qemuImgPath, + if (!(cmd = virCommandNewArgList("qemu-img", "rebase", "-u", "-F", @@ -3565,7 +3550,7 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); - qemuSnapshotUpdateBackingStore(vm, data); + qemuSnapshotUpdateBackingStore(data); if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0) goto error; -- 2.47.0

The functions are exclusively used in the snapshot module. Move and rename them: qemuDomainSnapshotForEachQcow2Raw -> qemuSnapshotForEachQcow2Internal qemuDomainSnapshotForEachQcow2 -> qemuSnapshotForEachQcow2 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 81 ------------------------------------ src/qemu/qemu_domain.h | 5 --- src/qemu/qemu_snapshot.c | 89 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 89 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5fdd7f9fc0..4c0bfcd172 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5705,87 +5705,6 @@ qemuDomainSnapshotWriteMetadata(virDomainObj *vm, } -/* 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 -qemuDomainSnapshotForEachQcow2Raw(virDomainDef *def, - virDomainMomentObj *snap, - const char *op, - bool try_all, - int ndisks) -{ - virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); - size_t i; - bool skipped = false; - - 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]); - - /* 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)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("can't manipulate inactive snapshots of disk '%1$s'"), - def->disks[i]->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. */ - qemuDomainSnapshotForEachQcow2Raw(def, snap, "-d", false, i); - } - virReportError(VIR_ERR_OPERATION_INVALID, - _("Disk device '%1$s' does not support snapshotting"), - def->disks[i]->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. */ - qemuDomainSnapshotForEachQcow2Raw(def, snap, "-d", false, i); - } - return -1; - } - } - - return skipped ? 1 : 0; -} - -/* The domain is expected to be locked and inactive. Return -1 on normal - * failure, 1 if we skipped a disk due to try_all. */ -int -qemuDomainSnapshotForEachQcow2(virDomainDef *def, - virDomainMomentObj *snap, - const char *op, - bool try_all) -{ - return qemuDomainSnapshotForEachQcow2Raw(def, snap, op, try_all, def->ndisks); -} - /* Hash iterator callback to discard multiple snapshots. */ int qemuDomainMomentDiscardAll(void *payload, const char *name G_GNUC_UNUSED, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ad0f4341c0..e810f79599 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -670,11 +670,6 @@ int qemuDomainSnapshotWriteMetadata(virDomainObj *vm, virDomainXMLOption *xmlopt, const char *snapshotDir); -int qemuDomainSnapshotForEachQcow2(virDomainDef *def, - virDomainMomentObj *snap, - const char *op, - bool try_all); - typedef struct _virQEMUMomentRemove virQEMUMomentRemove; struct _virQEMUMomentRemove { virQEMUDriver *driver; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f1d5103abe..222afe62e1 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -249,12 +249,95 @@ 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) +{ + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + size_t i; + bool skipped = false; + + 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]); + + /* 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)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't manipulate inactive snapshots of disk '%1$s'"), + def->disks[i]->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); + } + virReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%1$s' does not support snapshotting"), + def->disks[i]->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; + } + } + + return skipped ? 1 : 0; +} + + +/* 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); +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotCreateInactiveInternal(virDomainObj *vm, virDomainMomentObj *snap) { - return qemuDomainSnapshotForEachQcow2(vm->def, snap, "-c", false); + return qemuSnapshotForEachQcow2(vm->def, snap, "-c", false); } @@ -2551,7 +2634,7 @@ qemuSnapshotInternalRevertInactive(virDomainObj *vm, } /* Try all disks, but report failure if we skipped any. */ - if (qemuDomainSnapshotForEachQcow2(def, snap, "-a", true) != 0) + if (qemuSnapshotForEachQcow2(def, snap, "-a", true) != 0) return -1; return 0; @@ -3919,7 +4002,7 @@ qemuSnapshotDiscardImpl(virDomainObj *vm, if (qemuSnapshotDiscardExternal(vm, snap, externalData) < 0) return -1; } else { - if (qemuDomainSnapshotForEachQcow2(def, snap, "-d", true) < 0) + if (qemuSnapshotForEachQcow2(def, snap, "-d", true) < 0) return -1; } } else { -- 2.47.0

The live VM snapshot code already does handle the NVRAM image when it's in use, so we should also handle it when modifying/creating the snapshots via qemu-img when inactive. Add the handling to qemuSnapshotForEachQcow2 which is used for all inactive operations. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f560cf270c..4a17935627 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -347,6 +347,26 @@ qemuSnapshotForEachQcow2(virDomainDef *def, } } + if (def->os.loader && def->os.loader->nvram) { + virStorageSource *nvram = def->os.loader->nvram; + + if (virStorageSourceIsLocalStorage(nvram) && + nvram->format == VIR_STORAGE_FILE_QCOW2) { + if (qemuSnapshotForEachQcow2One(nvram, op, snap->def->name) < 0) { + if (create) { + nrollback = def->ndisks; + virErrorPreserveLast(&orig_err); + goto rollback; + } else { + VIR_WARN("failed 'qemu-img snapshot %s' action on NVRAM image", + op); + skipped = true; + virResetLastError(); + } + } + } + } + return skipped ? 1 : 0; rollback: -- 2.47.0

The internal snapshot code will use the 'snapshot-load' command so we need to add the corresponding job type. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 1 + src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_blockjob.h | 1 + src/qemu/qemu_domain.c | 2 ++ 4 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 692b4d350e..3c1305ec84 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3777,6 +3777,7 @@ qemuBlockPivot(virDomainObj *vm, case QEMU_BLOCKJOB_TYPE_CREATE: case QEMU_BLOCKJOB_TYPE_SNAPSHOT_SAVE: case QEMU_BLOCKJOB_TYPE_SNAPSHOT_DELETE: + case QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD: case QEMU_BLOCKJOB_TYPE_BROKEN: virReportError(VIR_ERR_OPERATION_INVALID, _("job type '%1$s' does not support pivot"), diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 6e53603fba..c35321790e 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -70,6 +70,7 @@ VIR_ENUM_IMPL(qemuBlockjob, "create", "snapshot-save", "snapshot-delete", + "snapshot-load", "broken"); static virClass *qemuBlockJobDataClass; @@ -1459,6 +1460,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobData *job, case QEMU_BLOCKJOB_TYPE_SNAPSHOT_SAVE: case QEMU_BLOCKJOB_TYPE_SNAPSHOT_DELETE: + case QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD: /* The internal snapshot jobs don't need any extra handling */ break; diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 6620e08c47..572a838676 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -67,6 +67,7 @@ typedef enum { QEMU_BLOCKJOB_TYPE_CREATE, QEMU_BLOCKJOB_TYPE_SNAPSHOT_SAVE, QEMU_BLOCKJOB_TYPE_SNAPSHOT_DELETE, + QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD, QEMU_BLOCKJOB_TYPE_BROKEN, QEMU_BLOCKJOB_TYPE_LAST } qemuBlockJobType; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4c0bfcd172..dfbf3dd9f0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2478,6 +2478,7 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload, case QEMU_BLOCKJOB_TYPE_SNAPSHOT_SAVE: case QEMU_BLOCKJOB_TYPE_SNAPSHOT_DELETE: + case QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD: /* No private data for internal snapshot jobs */ break; @@ -3035,6 +3036,7 @@ qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobData *job, case QEMU_BLOCKJOB_TYPE_SNAPSHOT_SAVE: case QEMU_BLOCKJOB_TYPE_SNAPSHOT_DELETE: + case QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD: /* No extra data for internal snapshot jobs. */ break; -- 2.47.0

The '-loadvm' commandline parameter has exactly the same semantics as the HMP 'loadvm' command. This includes the selection of which block device is considered to contain the 'vmstate' section. Since libvirt recently switched to the new QMP commands which allow a free selection of where the 'vmstate' is placed, snapshot reversion will no longer work if libvirt's algorithm disagrees with qemu's. This is the case when the VM has UEFI NVRAM image, in qcow2 format, present. To solve this we'll use the QMP counterpart 'snapshot-load' to load the snapshot instead of using '-loadvm'. We'll do this before resuming processors after startup of qemu and thus the behaviour is identical to what we had before. The logic for selecting the images now checks both the snapshot metadata and the VM definition. In case images not covered by the snapshot definition do have the snapshot it's included in the reversion, but it's fatal if the snapshot is not present in a disk covered in snapshot metadata. The vmstate is selected based on where it's present as libvirt doesn't store this information. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 5 +- src/qemu/qemu_process.c | 7 ++ src/qemu/qemu_snapshot.c | 232 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_snapshot.h | 5 + 4 files changed, 248 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 696f891b47..f4430275dc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10567,7 +10567,10 @@ qemuBuildCommandLine(virDomainObj *vm, if (qemuBuildSecCommandLine(vm, cmd, def->sec) < 0) return NULL; - if (snapshot) + /* Internal snapshot reversion happens via QMP command after startup if + * supported */ + if (snapshot && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SNAPSHOT_INTERNAL_QMP)) virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); if (def->namespaceData) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0815bffe3c..1fda8bdd65 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7944,6 +7944,13 @@ qemuProcessLaunch(virConnectPtr conn, qemuDomainVcpuPersistOrder(vm->def); + if (snapshot && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SNAPSHOT_INTERNAL_QMP)) { + VIR_DEBUG("reverting internal snapshot via QMP"); + if (qemuSnapshotInternalRevert(vm, snapshot, asyncJob) < 0) + goto cleanup; + } + VIR_DEBUG("Verifying and updating provided guest CPU"); if (qemuProcessUpdateAndVerifyCPU(vm, asyncJob) < 0) goto cleanup; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index aab06a09c6..5b3aadcbf0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -4361,3 +4361,235 @@ qemuSnapshotDelete(virDomainObj *vm, return ret; } + + +static char ** +qemuSnapshotInternalRevertGetDevices(virDomainObj *vm, + virDomainSnapshotDef *snapdef, + char **vmstate, + virDomainAsyncJob asyncJob) + +{ + g_autoptr(GHashTable) blockNamedNodeData = NULL; + const char *snapname = snapdef->parent.name; + g_auto(GStrv) devices = g_new0(char *, vm->def->ndisks + 2); + size_t ndevs = 0; + size_t i = 0; + const char *vmstate_candidate = NULL; + g_autoptr(GHashTable) snapdisks = virHashNew(NULL); + /* following variables add debug information */ + g_auto(virBuffer) errExtraSnap = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) errSnapWithoutMetadata = VIR_BUFFER_INITIALIZER; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + return NULL; + + /* Look up snapshot data from the snapshot object config itself */ + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = snapdef->disks + i; + virDomainDiskDef *domdisk = virDomainDiskByTarget(vm->def, snapdisk->name); + const char *format_nodename; + qemuBlockNamedNodeData *d = NULL; + qemuBlockNamedNodeDataSnapshot *sn = NULL; + + if (!domdisk) { + /* This can happen only if the snapshot metadata doesn't match the + * domain XML stored in the snapshot */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VM doesn't have disk '%1$s' referenced by snapshot '%2$s'"), + snapdisk->name, snapname); + return NULL; + } + + /* later we'll check if all disks from the VM definition XML were considered */ + g_hash_table_insert(snapdisks, g_strdup(snapdisk->name), NULL); + + format_nodename = qemuBlockStorageSourceGetFormatNodename(domdisk->src); + + /* Internal snapshots require image format which supports them, thus + * this effectively rejects any raw images */ + if (format_nodename) + d = g_hash_table_lookup(blockNamedNodeData, format_nodename); + + if (d && d->snapshots) + sn = g_hash_table_lookup(d->snapshots, snapname); + + if (sn) { + if (sn->vmstate) { + if (vmstate_candidate) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("two disks images contain vm state section for internal snapshot '%1$s'"), + snapname); + return NULL; + } + vmstate_candidate = format_nodename; + } + + devices[ndevs++] = g_strdup(format_nodename); + } + + switch (snapdisk->snapshot) { + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: + if (!sn) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("image of disk '%1$s' does not have internal snapshot '%2$s'"), + snapdisk->name, snapname); + return NULL; + } + + break; + + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_MANUAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NO: + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: + case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST: + if (sn) { + /* Unexpected internal snapshot present in image even if the + * snapshot metadata claims otherwise */ + virBufferAsprintf(&errExtraSnap, "%s ", snapdisk->name); + } + break; + } + } + + /* check if all VM disks were covered */ + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *domdisk = vm->def->disks[i]; + const char *format_nodename; + qemuBlockNamedNodeData *d = NULL; + qemuBlockNamedNodeDataSnapshot *sn = NULL; + + if (g_hash_table_contains(snapdisks, domdisk->dst)) + continue; + + format_nodename = qemuBlockStorageSourceGetFormatNodename(domdisk->src); + + if (format_nodename) + d = g_hash_table_lookup(blockNamedNodeData, format_nodename); + + if (d && d->snapshots) + sn = g_hash_table_lookup(d->snapshots, snapname); + + if (sn) { + virBufferAsprintf(&errSnapWithoutMetadata, "%s ", domdisk->dst); + + if (sn->vmstate) { + if (vmstate_candidate) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("two disks images contain vm state section for internal snapshot '%1$s'"), + snapname); + return NULL; + } + vmstate_candidate = format_nodename; + } + + devices[ndevs++] = g_strdup(format_nodename); + } + } + + /* pflash */ + if (vm->def->os.loader && + vm->def->os.loader->nvram && + vm->def->os.loader->nvram->format == VIR_STORAGE_FILE_QCOW2) { + const char *format_nodename; + qemuBlockNamedNodeData *d = NULL; + qemuBlockNamedNodeDataSnapshot *sn = NULL; + + if ((format_nodename = qemuBlockStorageSourceGetFormatNodename(vm->def->os.loader->nvram)) && + (d = virHashLookup(blockNamedNodeData, format_nodename)) && + d->snapshots && + (sn = g_hash_table_lookup(d->snapshots, snapname))) { + if (sn->vmstate) { + if (vmstate_candidate) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("two disks images contain vm state section for internal snapshot '%1$s'"), + snapname); + return NULL; + } + + vmstate_candidate = format_nodename; + } + + devices[ndevs++] = g_strdup(format_nodename); + } + } + + if (virBufferUse(&errExtraSnap) > 0 || + virBufferUse(&errSnapWithoutMetadata) > 0) { + VIR_WARN("inconsistent internal snapshot state (reversion): VM='%s' snapshot='%s' no-snapshot='%s' no-metadata='%s'", + vm->def->name, snapname, + virBufferCurrentContent(&errExtraSnap), + virBufferCurrentContent(&errSnapWithoutMetadata)); + } + + *vmstate = g_strdup(vmstate_candidate); + return g_steal_pointer(&devices); +} + + +int +qemuSnapshotInternalRevert(virDomainObj *vm, + virDomainMomentObj *snapshot, + virDomainAsyncJob asyncJob) +{ + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snapshot); + g_autofree char *jobname = g_strdup_printf("internal-snapshot-load-%s", snapdef->parent.name); + qemuBlockJobData *job = NULL; + g_auto(GStrv) devices = NULL; + g_autofree char *vmstate = NULL; + int rc = 0; + int ret = -1; + + if (!(devices = qemuSnapshotInternalRevertGetDevices(vm, snapdef, &vmstate, asyncJob))) + return -1; + + if (!vmstate) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("missing vmstate section when reverting active internal snapshot '%1$s'"), + snapshot->def->name); + return -1; + } + + if (!(job = qemuBlockJobDiskNew(vm, NULL, QEMU_BLOCKJOB_TYPE_SNAPSHOT_LOAD, + jobname))) + return -1; + + qemuBlockJobSyncBegin(job); + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + goto cleanup; + + rc = qemuMonitorSnapshotLoad(qemuDomainGetMonitor(vm), jobname, snapdef->parent.name, + vmstate, (const char **) devices); + qemuDomainObjExitMonitor(vm); + + if (rc < 0) + goto cleanup; + + qemuBlockJobStarted(job, vm); + + while (true) { + qemuBlockJobUpdate(vm, job, asyncJob); + + if (job->state == VIR_DOMAIN_BLOCK_JOB_FAILED) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("load of internal snapshot '%1$s' job failed: %2$s"), + snapdef->parent.name, NULLSTR(job->errmsg)); + goto cleanup; + } + + if (job->state == VIR_DOMAIN_BLOCK_JOB_COMPLETED) + break; + + if (qemuDomainObjWait(vm) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + qemuBlockJobStartupFinalize(vm, job); + + return ret; +} diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index 38437a2fd7..f38c2acfb3 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -84,3 +84,8 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt); virDomainSnapshotDiskDef * qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, const char *suffix); + +int +qemuSnapshotInternalRevert(virDomainObj *vm, + virDomainMomentObj *snapshot, + virDomainAsyncJob asyncJob); -- 2.47.0