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

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