On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote: Since the commit message is vague on the specific problem details ... ... I remember you talking about the need to create overlays also for images which might have been excluded in given snapshot (the one you are reverting to) , but externally snapshotted in a later (still existing) one. In such case not creating the overlay here would still invalidate the overlay of the next snapshot. Is that right? I also remember that you mentioned that the actual problem is with empty cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_saveimage.c | 17 ----------------- src/qemu/qemu_saveimage.h | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 41310d6a9a..eca47171c2 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -37,23 +37,6 @@ VIR_LOG_INIT("qemu.qemu_saveimage"); -typedef enum { - QEMU_SAVE_FORMAT_RAW = 0, - QEMU_SAVE_FORMAT_GZIP = 1, - QEMU_SAVE_FORMAT_BZIP2 = 2, - /* - * Deprecated by xz and never used as part of a release - * QEMU_SAVE_FORMAT_LZMA - */ - QEMU_SAVE_FORMAT_XZ = 3, - QEMU_SAVE_FORMAT_LZOP = 4, - /* Note: add new members only at the end. - These values are used in the on-disk format. - Do not change or re-use numbers. */ - - QEMU_SAVE_FORMAT_LAST -} virQEMUSaveFormat; - VIR_ENUM_DECL(qemuSaveCompression); VIR_ENUM_IMPL(qemuSaveCompression, QEMU_SAVE_FORMAT_LAST, diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 30cf4b1ee0..6892e6764f 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -32,6 +32,23 @@ G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL)); +typedef enum { + QEMU_SAVE_FORMAT_RAW = 0, + QEMU_SAVE_FORMAT_GZIP = 1, + QEMU_SAVE_FORMAT_BZIP2 = 2, + /* + * Deprecated by xz and never used as part of a release + * QEMU_SAVE_FORMAT_LZMA + */ + QEMU_SAVE_FORMAT_XZ = 3, + QEMU_SAVE_FORMAT_LZOP = 4, + /* Note: add new members only at the end. + These values are used in the on-disk format. + Do not change or re-use numbers. */ + + QEMU_SAVE_FORMAT_LAST +} virQEMUSaveFormat; + typedef struct _virQEMUSaveHeader virQEMUSaveHeader; struct _virQEMUSaveHeader { char magic[sizeof(QEMU_SAVE_MAGIC)-1]; -- 2.41.0

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_saveimage.c | 2 +- src/qemu/qemu_saveimage.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index eca47171c2..44ab263144 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -207,7 +207,7 @@ virQEMUSaveDataFinish(virQEMUSaveData *data, } -static virCommand * +virCommand * qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression) { virCommand *ret = NULL; diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 6892e6764f..510576baa2 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -105,6 +105,9 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, bool use_raw_on_fail) ATTRIBUTE_NONNULL(2); +virCommand * +qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression); + int qemuSaveImageCreate(virQEMUDriver *driver, virDomainObj *vm, -- 2.41.0

Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image. This duplicates some necessary parts of qemuSaveImageStartVM() because the external snapshot memory state is done by qemuSaveImageCreate(). Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 115 +++++++++++++++++++++++++++++++++------ 1 file changed, 97 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ff85d56572..538da6570a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2004,6 +2004,22 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +typedef struct _qemuSnapshotRevertMemoryData { + int fd; + char *path; + virQEMUSaveData *data; +} qemuSnapshotRevertMemoryData; + + +static void +qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata) +{ + VIR_FORCE_CLOSE(memdata->fd); + virQEMUSaveDataFree(memdata->data); +} +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, qemuSnapshotClearRevertMemoryData); + + /** * qemuSnapshotRevertExternalPrepare: * @vm: domain object @@ -2011,15 +2027,13 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, * @snap: snapshot object we are reverting to * @config: live domain definition * @inactiveConfig: offline domain definition - * memsnapFD: pointer to store memory state file FD or NULL - * memsnapPath: pointer to store memory state file path or NULL + * @memdata: struct with data to load memory state * * Prepare new temporary snapshot definition @tmpsnapdef that will * be used while creating new overlay files after reverting to snapshot * @snap. In case we are reverting to snapshot with memory state it will - * open it and pass FD via @memsnapFD and path to the file via - * @memsnapPath, caller is responsible for freeing both @memsnapFD and - * memsnapPath. + * open it and store necessary data in @memdata. Caller is responsible + * to clear the data by using qemuSnapshotClearRevertMemoryData(). * * Returns 0 in success, -1 on error. */ @@ -2029,8 +2043,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainMomentObj *snap, virDomainDef *config, virDomainDef *inactiveConfig, - int *memsnapFD, - char **memsnapPath) + qemuSnapshotRevertMemoryData *memdata) { size_t i; bool active = virDomainObjIsActive(vm); @@ -2065,12 +2078,20 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, return -1; } - if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + if (memdata && snapdef->memorysnapshotfile) { virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virDomainDef) savedef = NULL; - *memsnapPath = snapdef->memorysnapshotfile; - *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + memdata->path = snapdef->memorysnapshotfile; + memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path, + &savedef, &memdata->data, + false, NULL, false, false); + + if (memdata->fd < 0) + return -1; + + if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) + return -1; } return 0; @@ -2254,13 +2275,16 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; virDomainMomentObj *loadSnap = NULL; - VIR_AUTOCLOSE memsnapFD = -1; - char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL }; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *errbuf = NULL; + VIR_AUTOCLOSE intermediatefd = -1; + int memrc = 0; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -2284,7 +2308,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, *config, *inactiveConfig, - &memsnapFD, &memsnapPath) < 0) { + &memdata) < 0) { return -1; } } else { @@ -2298,6 +2322,30 @@ qemuSnapshotRevertActive(virDomainObj *vm, virDomainObjAssignDef(vm, config, true, NULL); + if (virDomainSnapshotIsExternal(snap) && memdata.data) { + virQEMUSaveHeader *header = &memdata.data->header; + + if (header && (header->version == 2) && + (header->compressed != QEMU_SAVE_FORMAT_RAW)) { + if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) + return -1; + + intermediatefd = memdata.fd; + memdata.fd = -1; + + virCommandSetInputFD(cmd, intermediatefd); + virCommandSetOutputFD(cmd, &memdata.fd); + virCommandSetErrorBuffer(cmd, &errbuf); + virCommandDoAsyncIO(cmd); + + if (virCommandRunAsync(cmd, NULL) < 0) { + memdata.fd = intermediatefd; + intermediatefd = -1; + return -1; + } + } + } + /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. */ @@ -2307,17 +2355,48 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_SNAPSHOT, NULL, memsnapFD, - memsnapPath, loadSnap, + VIR_ASYNC_JOB_SNAPSHOT, "stdio", + memdata.fd, memdata.path, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); + + if (virDomainSnapshotIsExternal(snap) && memdata.data) { + if (intermediatefd != -1) { + virErrorPtr orig_err = NULL; + + if (rc < 0) { + /* if there was an error setting up qemu, the intermediate + * process will wait forever to write to stdout, so we + * must manually kill it and ignore any error related to + * the process + */ + virErrorPreserveLast(&orig_err); + VIR_FORCE_CLOSE(intermediatefd); + VIR_FORCE_CLOSE(memdata.fd); + } + + memrc = virCommandWait(cmd, NULL); + VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf)); + virErrorRestore(&orig_err); + } + if (VIR_CLOSE(memdata.fd) < 0) { + virReportSystemError(errno, _("cannot close file: %1$s"), memdata.path); + memrc = -1; + } + + /* qemuProcessStart doesn't unset the qemu error reporting infrastructure + * in case of migration (which is used in this case) so we need to reset it + * so that the handle to virtlogd is not held open unnecessarily */ + qemuMonitorSetDomainLog(qemuDomainGetMonitor(vm), NULL, NULL, NULL); + } + virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, detail); virObjectEventStateQueue(driver->domainEventState, event); - if (rc < 0) + if (rc < 0 || memrc < 0) return -1; @@ -2428,7 +2507,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, NULL, *inactiveConfig, - NULL, NULL) < 0) { + NULL) < 0) { return -1; } -- 2.41.0

On Fri, Sep 01, 2023 at 10:32:15 +0200, Pavel Hrdina wrote: [...] As noted in review of v1, we ought to use the cookie from the save image or at least validate that this one is correct.