On 12/09/2016 02:27 AM, Nikolay Shirokovskiy wrote:
>
>
> On 08.12.2016 19:38, John Ferlan wrote:
>>
>>
>> On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
>>> In case of 0 filesystems *info is not set while according
>>> to virDomainGetFSInfo contract user should call free on it even
>>> in case of 0 filesystems. Thus we need to properly set
>>> it. NULL will be enough as free eats NULLs ok.
>>> ---
>>> src/qemu/qemu_agent.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>>> index ec8d47e..c5cf403 100644
>>> --- a/src/qemu/qemu_agent.c
>>> +++ b/src/qemu/qemu_agent.c
>>> @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>>> ndata = virJSONValueArraySize(data);
>>> if (!ndata) {
>>> ret = 0;
>>> + *info = NULL;
>>
>> ACK - although there are more ways above this hunk that allow us to get
>> to cleanup without setting *info = NULL; Currently each of the callers
>
> These are error paths. The caller have no obligations to free info in these cases.
>
>> sets the input info to NULL before calling here
>
> qemuDomainGetFSInfo does not set...
>
>>
>> IOW: We could also move that *info = NULL up before the call to
>> virAgentMakeCommand
>>
>
> I looked here and there in the libvirt code and found out that it does not zero out
> output parameter immediately. I guess it makes sense. Output parameter can be
> actually filled in deep below the call stack. Thus if one starts with convention
> to zero out immediately one have to do it in every function on the path.
I have a recollection of having it mentioned for one of my code reviews,
hence why setting *info = NULL unconditionally early on has stuck with
me... See virDomainGetIOThreadInfo.
Ironically virDomainGetFSInfo also has a *info = NULL prior to calling
domainGetFSInfo which calls into qemuDomainGetFSInfo.
As for testQemuAgentGetFSInfo (the other consumer of
qemuAgentGetFSInfo), it too has a qemuAgentFsInfoPtr *info = NULL; at
the top so well it should be happy, except that I see/note
qemuAgentGetFSInfo is called a second time, but in this case it expects
some sort of failure (it's not all that clear what's being tested, but
that's a different issue). In any case, prior to that call info was not
freed nor set to NULL (maybe that's the test case - I didn't think too
long and hard about it).
In any case, whether you decide in your v2 to put the *info at the top
or keep it where it is - doesn't matter to retain the ACK.
John
>
> I guess caller itself can zero out output parameter to simplify its cleanup logic.
> And some callers are not zero out, they cleanup conditionally for example - these
> rely on function to properly set output parameter.
>
> Nikolay
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
