On Tue, Jul 2, 2013 at 1:36 AM, Laszlo Ersek <ler...@redhat.com> wrote:
> On 07/01/13 19:59, Tomoki Sekiyama wrote:
>> On 7/1/13 9:29 , "Laszlo Ersek" <ler...@redhat.com> 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.
One thing to keep in mind is there are some failure paths in
qmp_guest_fsfreeze_thaw(), for both win32 and posix, where we might not unset
frozen state via ga_set_unfrozen(). In which case, logging will still be
disabled.
It might make sense to nest those error strings inside the one returned by
qmp_guest_fsfreeze_freeze(), since that's the only reliable way to report it
and the client will be interested in that information. This also makes
introducing local_err immediately useful.
>
> Thanks,
> Laszlo
>
