[PATCH 0/2] qemu: Improve opening and verifying save images
qemuSaveImageOpen does a lot of stuff. When bypass-cache is specified, it creates a virFileWrapperFd, which is not cleaned up in the subsequent failure paths. This results in errors from the iohelper, which can overwrite the actual error. E.g. consider libvirt attempting to restore an image saved in an unknown format # virsh restore --bypass-cache /data/test.sav.sparse error: Failed to restore domain from /data/test.sav.sparse error: internal error: Child process (LIBVIRT_LOG_OUTPUTS=1:stderr /usr/lib64/libvirt/libvirt_iohelper /data/test.sav.sparse 0) unexpected fatal signal 13 When not using the iohelper, and not creating a virFileWrapperFd, we see the real error: # virsh restore /data/test.sav.sparse error: Failed to restore domain from /data/test.sav.sparse error: operation failed: Invalid compressed save format 6 Although that error highlights a spot I missed when removing the "compression" implications around the 'foo_image_format' settings in qemu.conf with commit bd6d7ebf622 :-). IMO, the error message would be best fixed by checking for valid values when reading the save image metadata. Patch 1 decomposes qemuSaveImageOpen to allow for better error handling and more flexibility. Patch 2 checks for a valid format when checking the other header fields. Jim Fehlig (2): qemu: Decompose qemuSaveImageOpen qemu: Check for valid save image format when verifying image header src/qemu/qemu_driver.c | 37 +++++++-------- src/qemu/qemu_saveimage.c | 95 +++++++++++++++++++++++++-------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 95 insertions(+), 62 deletions(-) -- 2.43.0
Split the reading of libvirt's save image metadata from the opening of the fd that will be passed to QEMU. This allows improved error handling and provides more flexibility for code reading saved images. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 37 ++++++++-------- src/qemu/qemu_saveimage.c | 89 ++++++++++++++++++++++++--------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 89 insertions(+), 62 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2eddbd9ae..75de0b1fd5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5775,9 +5775,12 @@ qemuDomainRestoreInternal(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, + if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data, false) < 0) + goto cleanup; + + fd = qemuSaveImageOpen(driver, path, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &wrapperFd, false, false); + &wrapperFd, false); if (fd < 0) goto cleanup; @@ -5906,15 +5909,11 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virQEMUDriver *driver = conn->privateData; char *ret = NULL; g_autoptr(virDomainDef) def = NULL; - int fd = -1; virQEMUSaveData *data = NULL; virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, - false, NULL, false, false); - - if (fd < 0) + if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data, false) < 0) goto cleanup; if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0) @@ -5924,7 +5923,6 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, cleanup: virQEMUSaveDataFree(data); - VIR_FORCE_CLOSE(fd); return ret; } @@ -5948,8 +5946,10 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, else if (flags & VIR_DOMAIN_SAVE_PAUSED) state = 0; - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, - false, NULL, true, false); + if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data, false) < 0) + goto cleanup; + + fd = qemuSaveImageOpen(driver, path, 0, NULL, false); if (fd < 0) goto cleanup; @@ -6007,7 +6007,6 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) g_autofree char *path = NULL; char *ret = NULL; g_autoptr(virDomainDef) def = NULL; - int fd = -1; virQEMUSaveData *data = NULL; qemuDomainObjPrivate *priv; @@ -6029,15 +6028,14 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; } - if ((fd = qemuSaveImageOpen(driver, priv->qemuCaps, path, &def, &data, - false, NULL, false, false)) < 0) + if (qemuSaveImageGetMetadata(driver, priv->qemuCaps, path, + &def, &data, false) < 0) goto cleanup; ret = qemuDomainDefFormatXML(driver, priv->qemuCaps, def, flags); cleanup: virQEMUSaveDataFree(data); - VIR_FORCE_CLOSE(fd); virDomainObjEndAPI(&vm); return ret; } @@ -6093,14 +6091,17 @@ qemuDomainObjRestore(virConnectPtr conn, virQEMUSaveData *data = NULL; virFileWrapperFd *wrapperFd = NULL; - fd = qemuSaveImageOpen(driver, NULL, path, &def, &data, - bypass_cache, &wrapperFd, false, true); - if (fd < 0) { - if (fd == -3) + ret = qemuSaveImageGetMetadata(driver, NULL, path, &def, &data, true); + if (ret < 0) { + if (ret == -3) ret = 1; goto cleanup; } + fd = qemuSaveImageOpen(driver, path, bypass_cache, &wrapperFd, false); + if (fd < 0) + goto cleanup; + if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { int hookret; diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 69617e07eb..76d9e96792 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -522,58 +522,35 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, /** - * qemuSaveImageOpen: + * qemuSaveImageGetMetadata: * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header - * @bypass_cache: bypass cache when opening the file - * @wrapperFd: returns the file wrapper structure - * @open_write: open the file for writing (for updates) * @unlink_corrupt: remove the image file if it is corrupted * - * Returns the opened fd of the save image file and fills the appropriate fields - * on success. On error returns -1 on most failures, -3 if corrupt image was - * unlinked (no error raised). + * Open the save image file, read libvirt's save image metadata, and populate + * the @ret_def and @ret_data structures. Returns 0 on success and -1 on most + * failures. Returns -3 if corrupt image was unlinked (no error raised). */ int -qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, - const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, - bool bypass_cache, - virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; - int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeader *header; g_autoptr(virDomainDef) def = NULL; - int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len; - if (bypass_cache) { - int directFlag = virFileDirectFdFlag(); - if (directFlag < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("bypass cache unsupported by this system")); - return -1; - } - oflags |= directFlag; - } - - if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) - return -1; - - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) return -1; data = g_new0(virQEMUSaveData, 1); @@ -671,6 +648,50 @@ qemuSaveImageOpen(virQEMUDriver *driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); + return 0; +} + + +/** + * qemuSaveImageOpen: + * @driver: qemu driver data + * @path: path of the save image + * @bypass_cache: bypass cache when opening the file + * @wrapperFd: returns the file wrapper structure + * @open_write: open the file for writing (for updates) + * + * Returns the opened fd of the save image file on success, -1 on failure. + */ +int +qemuSaveImageOpen(virQEMUDriver *driver, + const char *path, + bool bypass_cache, + virFileWrapperFd **wrapperFd, + bool open_write) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOCLOSE fd = -1; + int ret = -1; + int oflags = open_write ? O_RDWR : O_RDONLY; + + if (bypass_cache) { + int directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + return -1; + } + oflags |= directFlag; + } + + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) + return -1; + + if (bypass_cache && + !(*wrapperFd = virFileWrapperFdNew(&fd, path, + VIR_FILE_WRAPPER_BYPASS_CACHE))) + return -1; + ret = fd; fd = -1; diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 0e58dd14b6..a3b9182258 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -69,17 +69,21 @@ qemuSaveImageStartVM(virConnectPtr conn, virDomainAsyncJob asyncJob) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6); +int +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); + int qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, bool bypass_cache, virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + bool open_write); int qemuSaveImageGetCompressionProgram(const char *imageFormat, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 80cd54bf33..e5c41fcf67 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2377,11 +2377,12 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, g_autoptr(virDomainDef) savedef = NULL; memdata->path = snapdef->memorysnapshotfile; - memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path, - &savedef, &memdata->data, - false, NULL, - false, false); + if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, &savedef, + &memdata->data, false) < 0) + return -1; + memdata->fd = qemuSaveImageOpen(driver, memdata->path, + false, NULL, false); if (memdata->fd < 0) return -1; -- 2.43.0
On 1/25/25 01:37, Jim Fehlig via Devel wrote:
Split the reading of libvirt's save image metadata from the opening of the fd that will be passed to QEMU. This allows improved error handling and provides more flexibility for code reading saved images.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 37 ++++++++-------- src/qemu/qemu_saveimage.c | 89 ++++++++++++++++++++++++--------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 89 insertions(+), 62 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 69617e07eb..76d9e96792 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -522,58 +522,35 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
/** - * qemuSaveImageOpen: + * qemuSaveImageGetMetadata: * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header - * @bypass_cache: bypass cache when opening the file - * @wrapperFd: returns the file wrapper structure - * @open_write: open the file for writing (for updates) * @unlink_corrupt: remove the image file if it is corrupted * - * Returns the opened fd of the save image file and fills the appropriate fields - * on success. On error returns -1 on most failures, -3 if corrupt image was - * unlinked (no error raised). + * Open the save image file, read libvirt's save image metadata, and populate + * the @ret_def and @ret_data structures. Returns 0 on success and -1 on most + * failures. Returns -3 if corrupt image was unlinked (no error raised). */ int -qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, - const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, - bool bypass_cache, - virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; - int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeader *header; g_autoptr(virDomainDef) def = NULL; - int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len;
- if (bypass_cache) { - int directFlag = virFileDirectFdFlag(); - if (directFlag < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("bypass cache unsupported by this system")); - return -1; - } - oflags |= directFlag; - } - - if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) - return -1; - - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) return -1;
data = g_new0(virQEMUSaveData, 1); @@ -671,6 +648,50 @@ qemuSaveImageOpen(virQEMUDriver *driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data);
+ return 0; +} + + +/** + * qemuSaveImageOpen: + * @driver: qemu driver data + * @path: path of the save image + * @bypass_cache: bypass cache when opening the file + * @wrapperFd: returns the file wrapper structure + * @open_write: open the file for writing (for updates) + * + * Returns the opened fd of the save image file on success, -1 on failure. + */ +int +qemuSaveImageOpen(virQEMUDriver *driver, + const char *path, + bool bypass_cache, + virFileWrapperFd **wrapperFd, + bool open_write) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOCLOSE fd = -1; + int ret = -1; + int oflags = open_write ? O_RDWR : O_RDONLY; + + if (bypass_cache) { + int directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + return -1; + } + oflags |= directFlag; + } + + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) + return -1;
I don't think it's this easy. Previously, the libvirt header was read and @fd was left at the beginning of QEMU migration stream. Now @fd points at the beginning of the file (our header). I wonder whether we can somehow pass @fd from qemuSaveImageGetMetadata() into here (and possibly reopen it for RW should @open_write be set).
+ + if (bypass_cache && + !(*wrapperFd = virFileWrapperFdNew(&fd, path, + VIR_FILE_WRAPPER_BYPASS_CACHE))) + return -1; + ret = fd; fd = -1;
Michal
On 1/28/25 06:52, Michal Prívozník wrote:
On 1/25/25 01:37, Jim Fehlig via Devel wrote:
Split the reading of libvirt's save image metadata from the opening of the fd that will be passed to QEMU. This allows improved error handling and provides more flexibility for code reading saved images.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 37 ++++++++-------- src/qemu/qemu_saveimage.c | 89 ++++++++++++++++++++++++--------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 89 insertions(+), 62 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 69617e07eb..76d9e96792 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -522,58 +522,35 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
/** - * qemuSaveImageOpen: + * qemuSaveImageGetMetadata: * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header - * @bypass_cache: bypass cache when opening the file - * @wrapperFd: returns the file wrapper structure - * @open_write: open the file for writing (for updates) * @unlink_corrupt: remove the image file if it is corrupted * - * Returns the opened fd of the save image file and fills the appropriate fields - * on success. On error returns -1 on most failures, -3 if corrupt image was - * unlinked (no error raised). + * Open the save image file, read libvirt's save image metadata, and populate + * the @ret_def and @ret_data structures. Returns 0 on success and -1 on most + * failures. Returns -3 if corrupt image was unlinked (no error raised). */ int -qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, - const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, - bool bypass_cache, - virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; - int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeader *header; g_autoptr(virDomainDef) def = NULL; - int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len;
- if (bypass_cache) { - int directFlag = virFileDirectFdFlag(); - if (directFlag < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("bypass cache unsupported by this system")); - return -1; - } - oflags |= directFlag; - } - - if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) - return -1; - - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) return -1;
data = g_new0(virQEMUSaveData, 1); @@ -671,6 +648,50 @@ qemuSaveImageOpen(virQEMUDriver *driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data);
+ return 0; +} + + +/** + * qemuSaveImageOpen: + * @driver: qemu driver data + * @path: path of the save image + * @bypass_cache: bypass cache when opening the file + * @wrapperFd: returns the file wrapper structure + * @open_write: open the file for writing (for updates) + * + * Returns the opened fd of the save image file on success, -1 on failure. + */ +int +qemuSaveImageOpen(virQEMUDriver *driver, + const char *path, + bool bypass_cache, + virFileWrapperFd **wrapperFd, + bool open_write) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOCLOSE fd = -1; + int ret = -1; + int oflags = open_write ? O_RDWR : O_RDONLY; + + if (bypass_cache) { + int directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + return -1; + } + oflags |= directFlag; + } + + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) + return -1;
I don't think it's this easy. Previously, the libvirt header was read and @fd was left at the beginning of QEMU migration stream. Now @fd points at the beginning of the file (our header).
Ah, right. I have a hack to handle that in patch 14/20 of the series this patch was lifted from. See the changes to qemuProcessIncomingDefNew https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/QB4O...
I wonder whether we can somehow pass @fd from qemuSaveImageGetMetadata() into here (and possibly reopen it for RW should @open_write be set).
What's your opinion on just reading the header again, as is done in the above patch? Would be nice to lseek to the proper position, but that doesn't work with virFileWrapperFd. Jim
On 1/28/25 22:23, Jim Fehlig wrote:
On 1/28/25 06:52, Michal Prívozník wrote:
On 1/25/25 01:37, Jim Fehlig via Devel wrote:
Split the reading of libvirt's save image metadata from the opening of the fd that will be passed to QEMU. This allows improved error handling and provides more flexibility for code reading saved images.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 37 ++++++++-------- src/qemu/qemu_saveimage.c | 89 ++++++++++++++++++++++++--------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 89 insertions(+), 62 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 69617e07eb..76d9e96792 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -522,58 +522,35 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, /** - * qemuSaveImageOpen: + * qemuSaveImageGetMetadata: * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header - * @bypass_cache: bypass cache when opening the file - * @wrapperFd: returns the file wrapper structure - * @open_write: open the file for writing (for updates) * @unlink_corrupt: remove the image file if it is corrupted * - * Returns the opened fd of the save image file and fills the appropriate fields - * on success. On error returns -1 on most failures, -3 if corrupt image was - * unlinked (no error raised). + * Open the save image file, read libvirt's save image metadata, and populate + * the @ret_def and @ret_data structures. Returns 0 on success and -1 on most + * failures. Returns -3 if corrupt image was unlinked (no error raised). */ int -qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, - const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, - bool bypass_cache, - virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; - int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeader *header; g_autoptr(virDomainDef) def = NULL; - int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len; - if (bypass_cache) { - int directFlag = virFileDirectFdFlag(); - if (directFlag < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("bypass cache unsupported by this system")); - return -1; - } - oflags |= directFlag; - } - - if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) - return -1; - - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) return -1; data = g_new0(virQEMUSaveData, 1); @@ -671,6 +648,50 @@ qemuSaveImageOpen(virQEMUDriver *driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); + return 0; +} + + +/** + * qemuSaveImageOpen: + * @driver: qemu driver data + * @path: path of the save image + * @bypass_cache: bypass cache when opening the file + * @wrapperFd: returns the file wrapper structure + * @open_write: open the file for writing (for updates) + * + * Returns the opened fd of the save image file on success, -1 on failure. + */ +int +qemuSaveImageOpen(virQEMUDriver *driver, + const char *path, + bool bypass_cache, + virFileWrapperFd **wrapperFd, + bool open_write) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOCLOSE fd = -1; + int ret = -1; + int oflags = open_write ? O_RDWR : O_RDONLY; + + if (bypass_cache) { + int directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + return -1; + } + oflags |= directFlag; + } + + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) + return -1;
I don't think it's this easy. Previously, the libvirt header was read and @fd was left at the beginning of QEMU migration stream. Now @fd points at the beginning of the file (our header).
Ah, right. I have a hack to handle that in patch 14/20 of the series this patch was lifted from. See the changes to qemuProcessIncomingDefNew
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/ QB4ORGCZHSXMC4RO6FGXMXTP5IJ5B5D4/
I wonder whether we can somehow pass @fd from qemuSaveImageGetMetadata() into here (and possibly reopen it for RW should @open_write be set).
What's your opinion on just reading the header again, as is done in the above patch? Would be nice to lseek to the proper position, but that doesn't work with virFileWrapperFd.
Ah right. Yeah, in that case the header needs to be read again. Alternatively, we might just use our patch 2/2 (should be merged regardless as it validates a field that wasn't validated), and then use virErrorPreserveLast() + virErrorRestore() combo like this: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a1fc61bae2..f38bbed198 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5766,6 +5766,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFd *wrapperFd = NULL; bool hook_taint = false; bool reset_nvram = false; + virErrorPtr orig_err; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -5837,6 +5838,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, qemuProcessEndJob(vm); cleanup: + virErrorPreserveLast(&orig_err); VIR_FORCE_CLOSE(fd); if (virFileWrapperFdClose(wrapperFd) < 0) ret = -1; @@ -5844,6 +5846,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virQEMUSaveDataFree(data); if (vm && ret < 0) qemuDomainRemoveInactive(driver, vm, 0, false); + virErrorRestore(&orig_err); virDomainObjEndAPI(&vm); return ret; } diff --git i/src/qemu/qemu_saveimage.c w/src/qemu/qemu_saveimage.c index 69617e07eb..d24f138bf4 100644 --- i/src/qemu/qemu_saveimage.c +++ w/src/qemu/qemu_saveimage.c @@ -631,6 +631,12 @@ qemuSaveImageOpen(virQEMUDriver *driver, return -1; } + if (header->format >= QEMU_SAVE_FORMAT_LAST) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("unsupported save image format: %1$d"), header->format); + return -1; + } + if (header->data_len <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("invalid header data length: %1$d"), header->data_len); This makes libvirt behave the same and saves us from opening the file twice. Obviously, I fixed just one location since it's just to demonstrate a point. Michal
On 1/29/25 04:40, Michal Prívozník wrote:
On 1/28/25 22:23, Jim Fehlig wrote:
On 1/28/25 06:52, Michal Prívozník wrote:
On 1/25/25 01:37, Jim Fehlig via Devel wrote:
Split the reading of libvirt's save image metadata from the opening of the fd that will be passed to QEMU. This allows improved error handling and provides more flexibility for code reading saved images.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 37 ++++++++-------- src/qemu/qemu_saveimage.c | 89 ++++++++++++++++++++++++--------------- src/qemu/qemu_saveimage.h | 16 ++++--- src/qemu/qemu_snapshot.c | 9 ++-- 4 files changed, 89 insertions(+), 62 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 69617e07eb..76d9e96792 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -522,58 +522,35 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, /** - * qemuSaveImageOpen: + * qemuSaveImageGetMetadata: * @driver: qemu driver data * @qemuCaps: pointer to qemuCaps if the domain is running or NULL * @path: path of the save image * @ret_def: returns domain definition created from the XML stored in the image * @ret_data: returns structure filled with data from the image header - * @bypass_cache: bypass cache when opening the file - * @wrapperFd: returns the file wrapper structure - * @open_write: open the file for writing (for updates) * @unlink_corrupt: remove the image file if it is corrupted * - * Returns the opened fd of the save image file and fills the appropriate fields - * on success. On error returns -1 on most failures, -3 if corrupt image was - * unlinked (no error raised). + * Open the save image file, read libvirt's save image metadata, and populate + * the @ret_def and @ret_data structures. Returns 0 on success and -1 on most + * failures. Returns -3 if corrupt image was unlinked (no error raised). */ int -qemuSaveImageOpen(virQEMUDriver *driver, - virQEMUCaps *qemuCaps, - const char *path, - virDomainDef **ret_def, - virQEMUSaveData **ret_data, - bool bypass_cache, - virFileWrapperFd **wrapperFd, - bool open_write, - bool unlink_corrupt) +qemuSaveImageGetMetadata(virQEMUDriver *driver, + virQEMUCaps *qemuCaps, + const char *path, + virDomainDef **ret_def, + virQEMUSaveData **ret_data, + bool unlink_corrupt) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; - int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; virQEMUSaveHeader *header; g_autoptr(virDomainDef) def = NULL; - int oflags = open_write ? O_RDWR : O_RDONLY; size_t xml_len; size_t cookie_len; - if (bypass_cache) { - int directFlag = virFileDirectFdFlag(); - if (directFlag < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("bypass cache unsupported by this system")); - return -1; - } - oflags |= directFlag; - } - - if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) - return -1; - - if (bypass_cache && - !(*wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, O_RDONLY, NULL)) < 0) return -1; data = g_new0(virQEMUSaveData, 1); @@ -671,6 +648,50 @@ qemuSaveImageOpen(virQEMUDriver *driver, *ret_def = g_steal_pointer(&def); *ret_data = g_steal_pointer(&data); + return 0; +} + + +/** + * qemuSaveImageOpen: + * @driver: qemu driver data + * @path: path of the save image + * @bypass_cache: bypass cache when opening the file + * @wrapperFd: returns the file wrapper structure + * @open_write: open the file for writing (for updates) + * + * Returns the opened fd of the save image file on success, -1 on failure. + */ +int +qemuSaveImageOpen(virQEMUDriver *driver, + const char *path, + bool bypass_cache, + virFileWrapperFd **wrapperFd, + bool open_write) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOCLOSE fd = -1; + int ret = -1; + int oflags = open_write ? O_RDWR : O_RDONLY; + + if (bypass_cache) { + int directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + return -1; + } + oflags |= directFlag; + } + + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) + return -1;
I don't think it's this easy. Previously, the libvirt header was read and @fd was left at the beginning of QEMU migration stream. Now @fd points at the beginning of the file (our header).
Ah, right. I have a hack to handle that in patch 14/20 of the series this patch was lifted from. See the changes to qemuProcessIncomingDefNew
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/ QB4ORGCZHSXMC4RO6FGXMXTP5IJ5B5D4/
I wonder whether we can somehow pass @fd from qemuSaveImageGetMetadata() into here (and possibly reopen it for RW should @open_write be set).
What's your opinion on just reading the header again, as is done in the above patch? Would be nice to lseek to the proper position, but that doesn't work with virFileWrapperFd.
Ah right. Yeah, in that case the header needs to be read again.
It's a tiny piece of the pie that shouldn't affect the overall restore time.
Alternatively, we might just use our patch 2/2 (should be merged regardless as it validates a field that wasn't validated), and then use virErrorPreserveLast() + virErrorRestore() combo like this:
I eventually need something like 1/2 for the mapped-ram support. Currently qemuSaveImageOpen does too much IMO. I've been working on an improvement to this series and would like to get opinions on that first. And before posting I'll be sure to test it on master, without the mapped-ram patches :-). Regards, Jim
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a1fc61bae2..f38bbed198 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5766,6 +5766,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFd *wrapperFd = NULL; bool hook_taint = false; bool reset_nvram = false; + virErrorPtr orig_err;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | VIR_DOMAIN_SAVE_RUNNING | @@ -5837,6 +5838,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, qemuProcessEndJob(vm);
cleanup: + virErrorPreserveLast(&orig_err); VIR_FORCE_CLOSE(fd); if (virFileWrapperFdClose(wrapperFd) < 0) ret = -1; @@ -5844,6 +5846,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virQEMUSaveDataFree(data); if (vm && ret < 0) qemuDomainRemoveInactive(driver, vm, 0, false); + virErrorRestore(&orig_err); virDomainObjEndAPI(&vm); return ret; } diff --git i/src/qemu/qemu_saveimage.c w/src/qemu/qemu_saveimage.c index 69617e07eb..d24f138bf4 100644 --- i/src/qemu/qemu_saveimage.c +++ w/src/qemu/qemu_saveimage.c @@ -631,6 +631,12 @@ qemuSaveImageOpen(virQEMUDriver *driver, return -1; }
+ if (header->format >= QEMU_SAVE_FORMAT_LAST) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("unsupported save image format: %1$d"), header->format); + return -1; + } + if (header->data_len <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("invalid header data length: %1$d"), header->data_len);
This makes libvirt behave the same and saves us from opening the file twice. Obviously, I fixed just one location since it's just to demonstrate a point.
Michal
On 1/29/25 18:35, Jim Fehlig wrote:
I eventually need something like 1/2 for the mapped-ram support. Currently qemuSaveImageOpen does too much IMO. I've been working on an improvement to this series and would like to get opinions on that first. And before posting I'll be sure to test it on master, without the mapped-ram patches :-).
I'm not against it. I'd need to look at mapped-ram patches, but I believe you. Go for it! Just make sure to test restore ;-) Michal
On 1/30/25 02:09, Michal Prívozník wrote:
On 1/29/25 18:35, Jim Fehlig wrote:
I eventually need something like 1/2 for the mapped-ram support. Currently qemuSaveImageOpen does too much IMO. I've been working on an improvement to this series and would like to get opinions on that first. And before posting I'll be sure to test it on master, without the mapped-ram patches :-).
I'm not against it. I'd need to look at mapped-ram patches, but I believe you. Go for it! Just make sure to test restore ;-)
Done now https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ECO25... I tested save/restore with and without bypass-cache, using compression, and managed save. Jim
When attempting to restore a saved image, the check for a valid save image format does not occur until the qemu process is about to be executed. Move the check earlier in the restore process, along with the other checks that verify a valid save image header. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_saveimage.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 76d9e96792..bcac6174ac 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -608,6 +608,12 @@ qemuSaveImageGetMetadata(virQEMUDriver *driver, return -1; } + if (header->format >= QEMU_SAVE_FORMAT_LAST) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("unsupported save image format: %1$d"), header->format); + return -1; + } + if (header->data_len <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("invalid header data length: %1$d"), header->data_len); -- 2.43.0
participants (2)
-
Jim Fehlig -
Michal Prívozník