Re: [libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo
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