[libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache

[Ani Sinha]

On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana <cfontana at suse.de> wrote:
>
> On 4/12/22 7:23 PM, Ani Sinha wrote:
> > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha <ani at anisinha.ca> wrote:
> >
> >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana <cfontana at suse.de> 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 at suse.de>
> >>> ---
> >>>  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;

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
> >>>
> >>
> >
>