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

[Claudio Fontana]

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?

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