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@xxxxxxxxxx] > Envoyé : mercredi 8 octobre 2014 18:24 > À : Yves Vinter; libvir-list@xxxxxxxxxx > Objet : Re: [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@xxxxxxxx> > > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list