6:37 p.m.
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
6:37 p.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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(a)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
7:52 a.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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(a)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
3:23 p.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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(a)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/Q...
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
5:40 a.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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(a)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
11:35 a.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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(a)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
3:09 a.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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
8:34 p.m.
New subject: [PATCH 1/2] qemu: Decompose qemuSaveImageOpen
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/EC...
I tested save/restore with and without bypass-cache, using compression, and
managed save.
Jim
6:37 p.m.
New subject: [PATCH 2/2] qemu: Check for valid save image format when verifying image header
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(a)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
5
days inactive
11
days old
8 comments
2 participants
participants (2)
-
Jim Fehlig -
Michal Prívozník