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@redhat.com]
Envoyé : mercredi 8 octobre 2014 18:24
À : Yves Vinter; libvir-list(a)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(a)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