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