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.0On 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/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.
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/ECO256ZTJDCRW76QHPYL6B5WXEKP5DDD/ I tested save/restore with and without bypass-cache, using compression, and managed save. Jim
© 2016 - 2025 Red Hat, Inc.