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

[Eric Blake]

On 10/09/2014 03:51 AM, Yves Vinter wrote:
> 1) Your git config is incorrect:
> 	OK; I fixed it
> 	Should I have to send again the whole set of patch?

At this point, you can wait for more reviews on the rest of the patches.
 But yes, at some point, you'll want to send a v2 of the series with all
the review changes incorporated.

> 
> 2) > +    virBufferFreeAndReset(&query);
> 
> 	Right; there are potential paths through which the query buffer may be not released (in hypervEnumAndPull).

Not if you take my alternate proposed 1/21 patch.

> 	For safety, I systematically added this line for any local virBuffer variable.

I'd rather fix the one function than have to fix every caller.

> 	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.

It's bad to have a function that releases resources only some of the
time.  It's better to either ALWAYS release the resources, and document
that the function consumes the argument, or to NEVER release the
resources, leaving it up to the caller.  Since the existing code is
already semi-relying on the function consuming the argument (such as
when a single hyperv_driver.c function reuses a query buffer twice in
the same function, possible because the first use of the query was reset
before building the second use of the buffer), my choice is to have the
query parameter always consumed, even on error.  Less code changes that way.

> 
> 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;...

It's okay, and even preferred, to interleave your replies (as I'm doing
here), rather than top-posting.  It makes it easier to follow the
conversation when it is interleaved.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141010/cf16df88/attachment-0001.sig>