On 4/13/22 8:57 AM, Ani Sinha wrote:
> 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.
>
It is a curious approach I agree, but I think this is how libvirt wants it, based on specifically this part of the comment in the code:
"
This function intentionally returns 0 when @wfd is NULL, so that
* callers can conditionally create a virFileWrapperFd wrapper but
* unconditionally call the cleanup code.
"
Thanks,
Claudio
