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@xxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxx/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
