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:[email protected]]
Envoyé : mercredi 8 octobre 2014 18:24
À : Yves Vinter; [email protected]
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 <[email protected]>
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
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
