8:35 p.m.
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