[PATCH 0/6] qemu: Avoid use of regular file FDs with qemu's 'fd:' migration backend
When reverting a save image in the most naive case libvirt would still try to use a regular file FD with the 'fd:' migration backend in qemu which will no longer work as of qemu-11.0. This patchet addresses it by using the filewrapper for all cases to pass pipes to 'fd' instead. In addition this patchset also adds a check that if 'sparse' save image format is used which requires the 'file' backend in qemu that a regular file FD is indeed passed to qemu. Peter Krempa (6): qemuSaveImageOpen: Remove wrong ATTRIBUTE_NONNULL qemuMonitorMigrateToFdSet: Drop 'flags' argument virfile: Introduce 'virFileFDIsRegular' qemuSaveImageCreateFd: Handle case when 'virQEMUFileOpenAs' doesn't return a file fd for 'sparse' format qemu: driver: Merge 'qemuDomainRestoreInternal' and 'qemuDomainObjRestore' qemu: saveimage: Use 'virFileWrapperFd' when loading non-sparse saveimage src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 254 +++++++++++++++++++------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_saveimage.c | 48 +++++-- src/qemu/qemu_saveimage.h | 6 +- src/qemu/qemu_snapshot.c | 12 +- src/util/virfile.c | 8 ++ src/util/virfile.h | 1 + 10 files changed, 191 insertions(+), 148 deletions(-) -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> After commit 517248e2394 removed the previously-4th argument the ATTRIBUTE_NONNULL(4) annotation no longer makes sense. Fixes: 517248e2394476a3105ff5866b0b718fc6583073 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_saveimage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index b46cabffe5..887545899f 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -95,7 +95,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, bool sparse, virFileWrapperFd **wrapperFd, bool open_write) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(2); int qemuSaveImageGetCompressionProgram(virQEMUSaveFormat format, -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> The only caller doesn't use it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 6 +++--- src/qemu/qemu_monitor.h | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0e097f25b1..fec808ccfb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -7219,7 +7219,7 @@ qemuMigrationSrcToSparseFile(virDomainObj *vm, if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) return -1; - ret = qemuMonitorMigrateToFdSet(vm, 0, fd, &directFd); + ret = qemuMonitorMigrateToFdSet(vm, fd, &directFd); qemuDomainObjExitMonitor(vm); return ret; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 958cd3a247..05334fabd5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2275,7 +2275,6 @@ qemuMonitorMigrateToFd(qemuMonitor *mon, int qemuMonitorMigrateToFdSet(virDomainObj *vm, - unsigned int flags, int *fd, int *directFd) { @@ -2284,9 +2283,10 @@ qemuMonitorMigrateToFdSet(virDomainObj *vm, off_t offset; g_autoptr(qemuFDPass) fdPassMigrate = NULL; g_autofree char *uri = NULL; + unsigned int migrateFlags = 0; /* currently no flags are passed to the 'migrate' command */ int ret; - VIR_DEBUG("fd=%d directFd=%d flags=0x%x", *fd, *directFd, flags); + VIR_DEBUG("fd=%d directFd=%d", *fd, *directFd); QEMU_CHECK_MONITOR(mon); @@ -2304,7 +2304,7 @@ qemuMonitorMigrateToFdSet(virDomainObj *vm, uri = g_strdup_printf("file:%s,offset=%#jx", qemuFDPassGetPath(fdPassMigrate), (uintmax_t)offset); - ret = qemuMonitorJSONMigrate(mon, flags, uri); + ret = qemuMonitorJSONMigrate(mon, migrateFlags, uri); return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ddbf3371ca..d3611d9713 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1090,7 +1090,6 @@ qemuMonitorMigrateToFd(qemuMonitor *mon, int qemuMonitorMigrateToFdSet(virDomainObj *vm, - unsigned int flags, int *fd, int *directFd); -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Similarly to 'virFileIsRegular' return if the FD is a regular file. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 8 ++++++++ src/util/virfile.h | 1 + 3 files changed, 10 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d8ae4f46cd..b200037189 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2375,6 +2375,7 @@ virFileDeleteTree; virFileDirectFdFlag; virFileExists; virFileFclose; +virFileFDIsRegular; virFileFdopen; virFileFindHugeTLBFS; virFileFindMountPoint; diff --git a/src/util/virfile.c b/src/util/virfile.c index 9316606ce8..65b04beb8c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2031,6 +2031,14 @@ virFileIsRegular(const char *path) } +bool +virFileFDIsRegular(int fd) +{ + struct stat s; + return (fstat(fd, &s) == 0) && S_ISREG(s.st_mode); +} + + /** * virFileExists: Check for presence of file * @path: Path of file to check diff --git a/src/util/virfile.h b/src/util/virfile.h index ce2ffb8ed4..8c9ad59898 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -218,6 +218,7 @@ bool virFileIsDir (const char *file) ATTRIBUTE_NONNULL(1); bool virFileExists(const char *file) ATTRIBUTE_NONNULL(1) ATTRIBUTE_MOCKABLE; bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1); bool virFileIsRegular(const char *file) ATTRIBUTE_NONNULL(1); +bool virFileFDIsRegular(int fd); enum { VIR_FILE_SHFS_NFS = (1 << 0), -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> The 'sparse' uses a mode in qemu which requires direct access to the file descriptior of the file itself. If we reside on root-squashed NFS the FD from 'virQEMUFileOpenAs' may not actually be a file which would not work with qemu. Reject such a config with a better error message and add documentation outlining the quirk. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_saveimage.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 23922acd51..a3519f8538 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -423,6 +423,20 @@ qemuSaveImageDecompressionStop(virCommand *cmd, } +/** + * qemuSaveImageCreateFd: + * @vm: domain object + * @path: path to the save image file + * @wrapperFd: filled with helper structure for the virFileWrapper + * @sparse: 'sparse' image format is used for the save image + * @needUnlink: if filled with 'true' a new file was created and needs to be + * removed on failure + * @bypassCache: Don't use cache for the writes of the save image + * + * Opens the save image @path and prepares it for saving of the VM state. + * Returns a file descriptor on succes. The returned FD is a pipe unless @ + * sparse is true in which case it's an fd to a file. + */ static int qemuSaveImageCreateFd(virDomainObj *vm, const char *path, @@ -455,6 +469,16 @@ qemuSaveImageCreateFd(virDomainObj *vm, if (fd < 0) return -1; + /* 'virQEMUFileOpenAs' can return a pipe/socket in case when it needs to bypass + * root-squashed NFS. Since 'sparse' backing format works only with real + * files we need to reject such cases */ + if (sparse && !virFileFDIsRegular(fd)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("path '%1$s' can't be opened directly (without the use of helper proces) which is incompatible with 'sparse' save image"), + path); + return -1; + } + if (qemuSecuritySetImageFDLabel(priv->driver->securityManager, vm->def, fd) < 0) return -1; -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> The two functions are for reverting a save image. They differ only on what domain object is used (new one vs existing one). Merge the code paths for existing VMs (for managed save restore) into 'qemuDomainRestoreInternal' and reuse it instead of 'qemuDomainObjRestore'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 222 +++++++++++++++++++---------------------- 1 file changed, 100 insertions(+), 122 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7f08fd05e9..c2d810c013 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5760,19 +5760,50 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn, return 0; } + +/** + * qemuDomainRestoreInternal: + * @conn: connection object + * @vmRestore: Domain object (optional; see below) + * @path: path to the save image file + * @unlink_corrupt: remove corrupted save image file @path + * @dxml: XML to replace definition in the save image (optional) + * @params: restore parameters + * @nparams: number of @params + * @flags: binary-OR of virDomainSaveRestoreFlags + * @ensureACL: callback for function checking ACL access (optional) + * @asyncJob: async job type + * + * Restores VM from save image at @path. + * + * If @vmRestore is non-NULL the VM object is reused and also the name and UUID + * of the VM from the save image is checked against it. + * + * @dxml can be used to optionally override the XML from the save image. + * + * @ensureACL must be passed unless the access to the domain object was already + * verified. + * + * Returns 0 on success; -1 on error. If @unlink_corrupt is true and the + * corrupted image was removed 1 is returned. + */ static int qemuDomainRestoreInternal(virConnectPtr conn, + virDomainObj *vmRestore, const char *path, + bool unlink_corrupt, const char *dxml, virTypedParameterPtr params, int nparams, unsigned int flags, - int (*ensureACL)(virConnectPtr, virDomainDef *)) + int (*ensureACL)(virConnectPtr, virDomainDef *), + virDomainAsyncJob asyncJob) { virQEMUDriver *driver = conn->privateData; qemuDomainObjPrivate *priv = NULL; g_autoptr(virDomainDef) def = NULL; virDomainObj *vm = NULL; + virDomainObj *vmNew = NULL; g_autofree char *xmlout = NULL; const char *newxml = dxml; int fd = -1; @@ -5792,8 +5823,21 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; - if (qemuSaveImageGetMetadata(driver, NULL, path, ensureACL, conn, &def, &data) < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, ensureACL, conn, &def, &data) < 0) { + if (unlink_corrupt && + qemuSaveImageIsCorrupt(driver, path)) { + if (unlink(path) < 0) { + virReportSystemError(errno, + _("cannot remove corrupt file: %1$s"), + path); + } else { + virResetLastError(); + ret = 1; + } + } + goto cleanup; + } sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; if (!(restoreParams = qemuMigrationParamsForSave(params, nparams, sparse, @@ -5833,12 +5877,36 @@ qemuDomainRestoreInternal(virConnectPtr conn, def = tmp; } - if (!(vm = virDomainObjListAdd(driver->domains, &def, - driver->xmlopt, - VIR_DOMAIN_OBJ_LIST_ADD_LIVE | - VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, - NULL))) - goto cleanup; + if (vmRestore) { + vm = vmRestore; + + if (STRNEQ(vm->def->name, def->name) || + memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { + char vm_uuidstr[VIR_UUID_STRING_BUFLEN]; + char def_uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, vm_uuidstr); + virUUIDFormat(def->uuid, def_uuidstr); + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot restore domain '%1$s' uuid %2$s from a file which belongs to domain '%3$s' uuid %4$s"), + vm->def->name, vm_uuidstr, + def->name, def_uuidstr); + goto cleanup; + } + + virDomainObjAssignDef(vm, &def, true, NULL); + } else { + if (!(vmNew = virDomainObjListAdd(driver->domains, &def, + driver->xmlopt, + VIR_DOMAIN_OBJ_LIST_ADD_LIVE | + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, + NULL))) + goto cleanup; + + vm = vmNew; + + if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_RESTORE, flags) < 0) + goto cleanup; + } if (flags & VIR_DOMAIN_SAVE_RUNNING) data->header.was_running = 1; @@ -5850,13 +5918,11 @@ qemuDomainRestoreInternal(virConnectPtr conn, priv->hookRun = true; } - if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_RESTORE, flags) < 0) - goto cleanup; - ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path, restoreParams, - false, reset_nvram, VIR_ASYNC_JOB_START); + false, reset_nvram, asyncJob); - qemuProcessEndJob(vm); + if (vmNew) + qemuProcessEndJob(vmNew); cleanup: VIR_FORCE_CLOSE(fd); @@ -5864,9 +5930,9 @@ qemuDomainRestoreInternal(virConnectPtr conn, ret = -1; virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); - if (vm && ret < 0) - qemuDomainRemoveInactive(vm, 0, false); - virDomainObjEndAPI(&vm); + if (vmNew && ret < 0) + qemuDomainRemoveInactive(vmNew, 0, false); + virDomainObjEndAPI(&vmNew); return ret; } @@ -5876,16 +5942,18 @@ qemuDomainRestoreFlags(virConnectPtr conn, const char *dxml, unsigned int flags) { - return qemuDomainRestoreInternal(conn, path, dxml, NULL, 0, flags, - virDomainRestoreFlagsEnsureACL); + return qemuDomainRestoreInternal(conn, NULL, path, false, dxml, NULL, 0, + flags, virDomainRestoreFlagsEnsureACL, + VIR_ASYNC_JOB_START); } static int qemuDomainRestore(virConnectPtr conn, const char *path) { - return qemuDomainRestoreInternal(conn, path, NULL, NULL, 0, 0, - virDomainRestoreEnsureACL); + return qemuDomainRestoreInternal(conn, NULL, path, false, NULL, NULL, 0, + 0, virDomainRestoreEnsureACL, + VIR_ASYNC_JOB_START); } static int @@ -5917,8 +5985,9 @@ qemuDomainRestoreParams(virConnectPtr conn, return -1; } - ret = qemuDomainRestoreInternal(conn, path, dxml, params, nparams, flags, - virDomainRestoreParamsEnsureACL); + ret = qemuDomainRestoreInternal(conn, NULL, path, false, dxml, params, nparams, + flags, virDomainRestoreParamsEnsureACL, + VIR_ASYNC_JOB_START); return ret; } @@ -6089,103 +6158,6 @@ qemuDomainManagedSaveDefineXML(virDomainPtr dom, const char *dxml, return ret; } -/* Return 0 on success, 1 if incomplete saved image was silently unlinked, - * and -1 on failure with error raised. */ -static int -qemuDomainObjRestore(virConnectPtr conn, - virQEMUDriver *driver, - virDomainObj *vm, - const char *path, - bool start_paused, - bool bypass_cache, - bool reset_nvram, - virDomainAsyncJob asyncJob) -{ - g_autoptr(virDomainDef) def = NULL; - qemuDomainObjPrivate *priv = vm->privateData; - int fd = -1; - int ret = -1; - g_autofree char *xmlout = NULL; - virQEMUSaveData *data = NULL; - virFileWrapperFd *wrapperFd = NULL; - bool sparse = false; - g_autoptr(qemuMigrationParams) restoreParams = NULL; - - ret = qemuSaveImageGetMetadata(driver, NULL, path, NULL, NULL, &def, &data); - if (ret < 0) { - if (qemuSaveImageIsCorrupt(driver, path)) { - if (unlink(path) < 0) { - virReportSystemError(errno, - _("cannot remove corrupt file: %1$s"), - path); - ret = -1; - } else { - virResetLastError(); - ret = 1; - } - } - goto cleanup; - } - - sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; - if (!(restoreParams = qemuMigrationParamsForSave(NULL, 0, sparse, - bypass_cache))) - return -1; - - fd = qemuSaveImageOpen(driver, path, bypass_cache, sparse, &wrapperFd, false); - if (fd < 0) - goto cleanup; - - if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { - int hookret; - - if ((hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, def->name, - VIR_HOOK_QEMU_OP_RESTORE, - VIR_HOOK_SUBOP_BEGIN, - NULL, data->xml, &xmlout)) < 0) - goto cleanup; - - if (hookret == 0 && !virStringIsEmpty(xmlout)) { - virDomainDef *tmp; - - VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); - - if (!(tmp = qemuSaveImageUpdateDef(driver, def, xmlout))) - goto cleanup; - - virDomainDefFree(def); - def = tmp; - priv->hookRun = true; - } - } - - if (STRNEQ(vm->def->name, def->name) || - memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { - char vm_uuidstr[VIR_UUID_STRING_BUFLEN]; - char def_uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, vm_uuidstr); - virUUIDFormat(def->uuid, def_uuidstr); - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot restore domain '%1$s' uuid %2$s from a file which belongs to domain '%3$s' uuid %4$s"), - vm->def->name, vm_uuidstr, - def->name, def_uuidstr); - goto cleanup; - } - - virDomainObjAssignDef(vm, &def, true, NULL); - - ret = qemuSaveImageStartVM(conn, driver, vm, &fd, data, path, restoreParams, - start_paused, reset_nvram, asyncJob); - - cleanup: - virQEMUSaveDataFree(data); - VIR_FORCE_CLOSE(fd); - if (virFileWrapperFdClose(wrapperFd) < 0) - ret = -1; - virFileWrapperFdFree(wrapperFd); - return ret; -} - static char *qemuDomainGetXMLDesc(virDomainPtr dom, @@ -6358,11 +6330,17 @@ qemuDomainObjStart(virConnectPtr conn, vm->hasManagedSave = false; } else { virDomainJobOperation op = vm->job->current->operation; + unsigned int restore_flags = 0; + vm->job->current->operation = VIR_DOMAIN_JOB_OPERATION_RESTORE; - ret = qemuDomainObjRestore(conn, driver, vm, managed_save, - start_paused, bypass_cache, - reset_nvram, asyncJob); + restore_flags |= start_paused ? VIR_DOMAIN_SAVE_PAUSED : 0; + restore_flags |= bypass_cache ? VIR_DOMAIN_SAVE_BYPASS_CACHE : 0; + restore_flags |= reset_nvram ? VIR_DOMAIN_SAVE_RESET_NVRAM : 0; + + ret = qemuDomainRestoreInternal(conn, vm, managed_save, true, NULL, NULL, 0, + restore_flags, NULL, + asyncJob); if (ret == 0) { if (unlink(managed_save) < 0) -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Always instantiate a 'virFileWrapperFd' (iohelper) to wrap the saveimage file descriptor of a non-'sparse' format saveimage. For 'sparse' images we also need to ensure that the FD returned when opening the save image is an actual file FD (thus not the FD from the helper process used to bypass root-squashed NFS) as qemu requires an actual file in those cases. This patch reworks 'qemuSaveImageOpen' to create the wrapper process based on whether the 'wrapperFd' variable is non-NULL rather than based on a combination of 'sparse' and 'bypass_cache' flags. The caller will then based on the image format and the need for the wrapper use the appropriate settings. As with this patch all non-sparse images will always pass a pipe instead of a file to qemu it also fixes problems with qemu-11.0 where the 'fd' migration protocol rejects FDs which point to a file. Resolves: https://issues.redhat.com/browse/RHEL-76301 Closes: https://gitlab.com/libvirt/libvirt/-/issues/850 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 32 ++++++++++++++++++++++++++------ src/qemu/qemu_saveimage.c | 24 +++++++++++++----------- src/qemu/qemu_saveimage.h | 4 +++- src/qemu/qemu_snapshot.c | 12 ++++++++++-- 4 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c2d810c013..03cdbd9dc3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5813,6 +5813,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, bool hook_taint = false; bool reset_nvram = false; bool sparse = false; + bool bypass_cache = (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0; g_autoptr(qemuMigrationParams) restoreParams = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -5844,11 +5845,30 @@ qemuDomainRestoreInternal(virConnectPtr conn, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)))) goto cleanup; - fd = qemuSaveImageOpen(driver, path, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - sparse, &wrapperFd, false); - if (fd < 0) - goto cleanup; + if (sparse) { + if ((fd = qemuSaveImageOpen(driver, path, bypass_cache, NULL, false)) < 0) + goto cleanup; + + /* In sparse mode the FD needs to be a real file as qemu accesses it + * directly thus: + * - ensure that we actualy got a file FD + * - there's no need to seek it as qemu does it itself + */ + if (!virFileFDIsRegular(fd)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("path '%1$s' can't be opened directly (without the use of helper proces) which is incompatible with 'sparse' save image"), + path); + goto cleanup; + } + } else { + if ((fd = qemuSaveImageOpen(driver, path, bypass_cache, &wrapperFd, false)) < 0) + goto cleanup; + + /* When virFileWrapperFD is used 'fd' can't be seeked so to make it + * point to the data read the header */ + if (qemuSaveImageFDSkipHeader(fd) < 0) + goto cleanup; + } if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { int hookret; @@ -6039,7 +6059,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, conn, &def, &data) < 0) goto cleanup; - fd = qemuSaveImageOpen(driver, path, false, false, NULL, true); + fd = qemuSaveImageOpen(driver, path, false, NULL, true); if (fd < 0) goto cleanup; diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index a3519f8538..64fbcd5f51 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -312,6 +312,13 @@ qemuSaveImageReadHeader(int fd, virQEMUSaveData **ret_data) } +int +qemuSaveImageFDSkipHeader(int fd) +{ + return qemuSaveImageReadHeader(fd, NULL); +} + + /** * qemuSaveImageDecompressionStart: * @data: data from memory state file @@ -697,7 +704,6 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, * @driver: qemu driver data * @path: path of the save image * @bypass_cache: bypass cache when opening the file - * @sparse: Image contains mapped-ram save format * @wrapperFd: returns the file wrapper structure * @open_write: open the file for writing (for updates) * @@ -707,7 +713,6 @@ int qemuSaveImageOpen(virQEMUDriver *driver, const char *path, bool bypass_cache, - bool sparse, virFileWrapperFd **wrapperFd, bool open_write) { @@ -729,16 +734,13 @@ qemuSaveImageOpen(virQEMUDriver *driver, if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) return -1; - /* If sparse, no need for the iohelper or positioning the file pointer. */ - if (!sparse) { - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) - return -1; + if (wrapperFd) { + unsigned int fdflags = VIR_FILE_WRAPPER_NON_BLOCKING; + + if (bypass_cache) + fdflags |= VIR_FILE_WRAPPER_BYPASS_CACHE; - /* Read the header to position the file pointer for QEMU. Unfortunately we - * can't use lseek with virFileWrapperFD. */ - if (qemuSaveImageReadHeader(fd, NULL) < 0) + if (!(*wrapperFd = virFileWrapperFdNew(&fd, path, fdflags))) return -1; } diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 887545899f..0d5a383212 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -92,7 +92,6 @@ int qemuSaveImageOpen(virQEMUDriver *driver, const char *path, bool bypass_cache, - bool sparse, virFileWrapperFd **wrapperFd, bool open_write) ATTRIBUTE_NONNULL(2); @@ -127,6 +126,9 @@ qemuSaveImageCreate(virDomainObj *vm, unsigned int flags, virDomainAsyncJob asyncJob); +int +qemuSaveImageFDSkipHeader(int fd); + int virQEMUSaveDataWrite(virQEMUSaveData *data, int fd, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 70e4c37144..82ae38ca29 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2423,6 +2423,7 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, typedef struct _qemuSnapshotRevertMemoryData { int fd; char *path; + virFileWrapperFd *wrapperFd; virQEMUSaveData *data; } qemuSnapshotRevertMemoryData; @@ -2430,6 +2431,8 @@ static void qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata) { VIR_FORCE_CLOSE(memdata->fd); + ignore_value(virFileWrapperFdClose(memdata->wrapperFd)); + virFileWrapperFdFree(memdata->wrapperFd); virQEMUSaveDataFree(memdata->data); } G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, qemuSnapshotClearRevertMemoryData); @@ -2506,10 +2509,15 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, return -1; memdata->fd = qemuSaveImageOpen(driver, memdata->path, - false, false, NULL, false); + false, &memdata->wrapperFd, false); if (memdata->fd < 0) return -1; + /* If 'wrapperFd' is used the FD can't be seeked so we need to make + * it point to the actual data, thus seek across the header */ + if (qemuSaveImageFDSkipHeader(memdata->fd) < 0) + return -1; + if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) return -1; } @@ -2705,7 +2713,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, bool defined = false; int rc; g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; - g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL }; + g_auto(qemuSnapshotRevertMemoryData) memdata = { .fd = -1 }; bool started = false; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; -- 2.53.0
On 2/17/26 16:52, Peter Krempa via Devel wrote:
When reverting a save image in the most naive case libvirt would still try to use a regular file FD with the 'fd:' migration backend in qemu which will no longer work as of qemu-11.0.
This patchet addresses it by using the filewrapper for all cases to pass pipes to 'fd' instead.
In addition this patchset also adds a check that if 'sparse' save image format is used which requires the 'file' backend in qemu that a regular file FD is indeed passed to qemu.
Peter Krempa (6): qemuSaveImageOpen: Remove wrong ATTRIBUTE_NONNULL qemuMonitorMigrateToFdSet: Drop 'flags' argument virfile: Introduce 'virFileFDIsRegular' qemuSaveImageCreateFd: Handle case when 'virQEMUFileOpenAs' doesn't return a file fd for 'sparse' format qemu: driver: Merge 'qemuDomainRestoreInternal' and 'qemuDomainObjRestore' qemu: saveimage: Use 'virFileWrapperFd' when loading non-sparse saveimage
src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 254 +++++++++++++++++++------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_saveimage.c | 48 +++++-- src/qemu/qemu_saveimage.h | 6 +- src/qemu/qemu_snapshot.c | 12 +- src/util/virfile.c | 8 ++ src/util/virfile.h | 1 + 10 files changed, 191 insertions(+), 148 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa