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

[Claudio Fontana]

On 4/13/22 6:13 AM, Ani Sinha wrote:
> 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.
>>

Hi Ani, did you read the comments snippet above?

To me it looks like there is nothing else to do here, because someone thought about this,
and made the wrapper API easier to use.

Thanks,

Claudio