On Wed, Apr 13, 2022 at 9:43 AM Ani Sinha <ani@xxxxxxxxxxx> wrote:
>
> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana@xxxxxxx> wrote:
> >
> > On 4/12/22 7:23 PM, Ani Sinha wrote:
> > > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani@xxxxxxxxxxx> wrote:
> > >
> > >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana@xxxxxxx> wrote:
> > >>>
> > >>> align the "save" with the "restore" code,
> > >>> by only using the wrapper when using --bypass-cache.
> > >>>
> > >>> This avoids a copy, resulting in better performance.
> > >>>
> > >>> Signed-off-by: Claudio Fontana <cfontana@xxxxxxx>
> > >>> ---
> > >>> src/qemu/qemu_saveimage.c | 6 ++++--
> > >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> > >>> index 4fd4c5cfcd..5ea1b2fbcc 100644
> > >>> --- a/src/qemu/qemu_saveimage.c
> > >>> +++ b/src/qemu/qemu_saveimage.c
> > >>> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> > >>> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
> > >> fd) < 0)
> > >>> goto cleanup;
> > >>>
> > >>> - if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> > >>> - goto cleanup;
> > >>> + if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> > >>> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> > >>> + goto cleanup;
> > >>> + }
> > >>
> > >> There is an obvious issue with this code. We are trying to close and
> > >> free a file descriptor that we have not opened when
> > >> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> > >
> > >
> > > I meant *not* set in flags.
> >
> > I don't think so. I don't think it is obvious, so can you be more specific?
>
> See
> if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> goto cleanup;
>
> and under cleanup:
> if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> ret = -1;
Also see virFileWrapperFdFree() in cleanup.
>
> if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> function calls use wrapperFd initialized to NULL.
> I think this patch would be incomplete if you do not handle all these scenarios.
>
> >
> > From the function header comments it seems lecit and defined to call the function with NULL:
> >
> > /**
> > * virFileWrapperFdClose:
> > * @wfd: fd wrapper, or NULL
> > *
> > * If @wfd is valid, then ensure that I/O has completed, which may
> > * include reaping a child process. Return 0 if all data for the
> > * wrapped fd is complete, or -1 on failure with an error emitted.
> > * This function intentionally returns 0 when @wfd is NULL, so that
> > * callers can conditionally create a virFileWrapperFd wrapper but
> > * unconditionally call the cleanup code. To avoid deadlock, only
> > * call this after closing the fd resulting from virFileWrapperFdNew().
> > *
> > * This function can be safely called multiple times on the same @wfd.
> > */
> >
> > Seems it has been specifically designed to simplify situations like this.
> >
> > Thanks,
> >
> > Claudio
> >
> >
> > >
> > >
> > >>
> > >>>
> > >>> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> > >>> goto cleanup;
> > >>> --
> > >>> 2.34.1
> > >>>
> > >>
> > >
> >
