[libvirt] [PATCH 01/21] Added missing virBufferFreeAndReset on the query buffer to free some memory

[Yves Vinter]

1) Your git config is incorrect:
	OK; I fixed it
	Should I have to send again the whole set of patch?

2) > +    virBufferFreeAndReset(&query);

	Right; there are potential paths through which the query buffer may be not released (in hypervEnumAndPull).
	For safety, I systematically added this line for any local virBuffer variable.
	I did the same for the new implemented features:
		(1) that indirectly uses the function hypervEnumAndPull
		(2) that uses the new hypervInvokeMethod to invoke WMI methods with complex parameters (after patch #12)
	Note that in case (2) hypervInvokeMethod don't release query buffers (those contained in EPR structures)
	The code could also be modified to avoid the callers to have to release their buffers.

3) I'm going to reply to this mail with an alternative shorter patch.  I don't have access to hyperv to test it under an actual valgrind run, but it compiled for me.  Can you please run it through your test setup to see if it solves the issues you were initially trying to address?

	Yes I can.
 
De : Eric Blake [mailto:eblake at redhat.com] 
Envoyé : mercredi 8 octobre 2014 18:24
À : Yves Vinter; libvir-list at redhat.com
Objet : Re: [libvirt] [PATCH 01/21] Added missing virBufferFreeAndReset on the query buffer to free some memory

On 10/08/2014 06:33 AM, Yves Vinter wrote:
> From: yvinter <yves.vinter at bull.net>

Your git config is incorrect; your email headers list your full name 'Yves Vinter' but this line means that git is trying to attribute to the authorship to your username 'yvinter'.  We prefer that the git history include a full legal name rather than a username abbreviation.

Long subject line; the first line of a commit message should generally be no more than about 60 characters, so that 'git log --oneline -30'
still fits the information comfortably in an 80-column screen.  Also, it is nice to include a 'topic:' lead-in.  Ideally, the one-line summary is the "what", and then the rest of the commit message after a blank line is the "why".  So I suggest:

hyperv: avoid memory leaks

Add missing virBufferFreeAndReset on query buffers used throughout the hyperv code.

as well as mention any valgrind testing you did to prove that the leaks are fixed.

> +++ b/src/hyperv/hyperv_driver.c
> @@ -201,6 +201,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags
>      VIR_FREE(username);
>      VIR_FREE(password);
>      hypervFreeObject(priv, (hypervObject *)computerSystem);
> +    virBufferFreeAndReset(&query);

Hmmm.  query is initialized empty, until it is used in this code a few lines earlier:


    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT);
    virBufferAddLit(&query, "where ");
    virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_PHYSICAL);

    if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) <
0) {
        goto cleanup;

but tracing through that code, hypervGetMsvmComputerSystemList in hyperv_wmi.generated.c calls hypervEnumAndPull in hyperv_wmi.c, which in turn calls virBufferContentAndReset(query) on the success path, leaving nothing to clean up here.  Okay, I see where hypervEnumAndPull can return early without cleaning query; I wonder if it would have been better to patch THAT function to guarantee that the buffer is always clean on return, rather than having to patch every single caller.

In fact, after looking through the entire patch, it looks like EVERY single addition of virBufferFreeAndReset(&query) is only ever useful in any case where hypervEnumAndPull returns early.

I'm going to reply to this mail with an alternative shorter patch.  I don't have access to hyperv to test it under an actual valgrind run, but it compiled for me.  Can you please run it through your test setup to see if it solves the issues you were initially trying to address?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org