Re: [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&gt; > --- > 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).