On Wed, Apr 13, 2022 at 11:57 AM Claudio Fontana <cfontana@xxxxxxx> wrote:
>
> On 4/13/22 6:13 AM, Ani Sinha 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;
> >
> > 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.
> >>
>
> Hi Ani, did you read the comments snippet above?
yes and I get that the called functions today are written with safety
for such cases in mind (check for null file descriptors). However, I
still think it is quite unclean (and looks buggy) to do stuff that
does not apply. With this change, wrapperFd only used when
VIR_DOMAIN_SAVE_BYPASS_CACHE is set and we should avoid any operations
on the fd when VIR_DOMAIN_SAVE_BYPASS_CACHE is not set.
That being said, previous to this patch, under error conditions from
call to virFileWrapperFdNew() also would call close() and free()
without checks for null fd from cleanup. So I guess we are not at
least making things any worse by not checking for null fd from the
caller of those two functions.
