Re: [Qemu-devel] [PATCH v4 08/10] qemu-ga: call Windows VSS requester in fsfreeze command handler

[Laszlo Ersek]

On 07/02/13 17:16, Luiz Capitulino wrote:
> On Tue, 02 Jul 2013 16:44:55 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 07/02/13 16:09, Luiz Capitulino wrote:
>>> On Tue, 02 Jul 2013 08:36:11 +0200
>>> Laszlo Ersek <address@hidden> wrote:
>>>
>>>> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>>>>> On 7/1/13 9:29 , "Laszlo Ersek" <address@hidden> wrote:
>>>>>
>>>>>>> +error:
>>>>>>> +    qmp_guest_fsfreeze_thaw(NULL);
>>>>>>
>>>>>> Passing NULL here as "errp" concerns me slightly. I've been assuming
>>>>>> that "errp" is never NULL due to the outermost QMP layer always wanting
>>>>>> to receive errors and to serialize them.
>>>>>>
>>>>>> Specifically, a NULL "errp" would turn all error_set*(errp, ...) calls
>>>>>> into no-ops under qmp_guest_fsfreeze_thaw(), and all error_is_set(errp)
>>>>>> questions would answer with false. That said, nothing seems to be
>>>>>> affected right now.
>>>>>>
>>>>>> Maybe a dummy local variable would be more future-proof... OTOH it would
>>>>>> be stylistically questionable :)
>>>>>
>>>>> OK, then it should be:
>>>>>     Error *local_err = NULL;
>>>>> ...
>>>>> error:
>>>>>     qmp_guest_fsfreeze_thaw(&local_err);
>>>>>     if (error_is_set(&local_err)) {
>>>>>         error_free(local_err);
>>>>>     }
>>>>
>>>> I think so, yes.
>>>>
>>>> You could also log it before freeing it I guess:
>>>>
>>>>     g_debug("cleanup thaw: %s", error_get_pretty(local_err));
>>>>
>>>> or some such, but it's probably not important.
>>>
>>> I'd rather do something like that, otherwise it doesn't seem to
>>> make sense to pass Error as qmp_guest_fsfreeze_thaw() does
>>> use a local Error.
>>
>> No, the win32 version of qmp_guest_fsfreeze_thaw() being added by this
>> patch doesn't.
> 
> Oh, I looked into the POSIX one. But then, it's qmp_guest_fsfreeze_thaw()
> that has to be fixed, isn't it?

I didn't insist on that because the QMP command implementations in the
guest agent are all rooted in the dispatcher, and the dispatcher makes
sure for all commands that errp is never NULL. This is the only call
site where a NULL errp was manually passed, and we're discussing how to
fix it -- use a local_err, and maybe even print it.

Of course I don't insist on *not* reworking it either.

Thanks,
Laszlo