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

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. > > Thanks, > > Claudio > > > > > > > >> > >>> > >>> if (virQEMUSaveDataWrite(data, fd, path) < 0) > >>> goto cleanup; > >>> -- > >>> 2.34.1 > >>> > >> > > >