On Thu, Aug 31, 2023 at 16:55:05 +0200, Pavel Hrdina wrote:
> 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.
>
> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
> ---
> src/qemu/qemu_snapshot.c | 78 ++++++++++++++++++++++++----------------
> 1 file changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index ff85d56572..869b46a9a8 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2004,6 +2004,21 @@ 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 +2026,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 +2042,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 +2077,21 @@ 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,13 @@ 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 };
> + bool started = false;
>
> start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
>
> @@ -2284,7 +2305,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
>
> if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap,
> *config, *inactiveConfig,
> - &memsnapFD, &memsnapPath) < 0)
> {
> + &memdata) < 0) {
> return -1;
> }
> } else {
> @@ -2298,28 +2319,24 @@ qemuSnapshotRevertActive(virDomainObj *vm,
>
> virDomainObjAssignDef(vm, config, true, NULL);
>
> - /* No cookie means libvirt which saved the domain was too old to
> - * mess up the CPU definitions.
> - */
> - if (cookie &&
> - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
> + if (qemuSaveImageStartProcess(snapshot->domain->conn, driver, vm,
> + &memdata.fd, memdata.path, loadSnap,
> + memdata.data ? &memdata.data->header :
> NULL,
> + cookie, VIR_ASYNC_JOB_SNAPSHOT,
> start_flags,
> + "from-snapshot", &started) < 0) {
I conceptually do not like the use of a function whose name implies that
a save image is used to also start qemu without a save image.
