Re: [libvirt] [PATCH 3/3] virBuffer: Try harder to free buffer

On 5/3/19 11:01 AM, Erik Skultety wrote: > On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote: > > Currently, the way virBufferFreeAndReset() works is it relies on > > virBufferContentAndReset() to fetch the buffer content which is > > then freed. This works as long as there is no bug in virBuffer* > > implementation (not true apparently). Explicitly call free() over > > buffer content. > > > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > --- > > src/util/virbuffer.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c > > index b2ae7963a1..ac03b15a61 100644 > > --- a/src/util/virbuffer.c > > +++ b/src/util/virbuffer.c > > @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf) > > */ > > void virBufferFreeAndReset(virBufferPtr buf) > > { > > - char *str = virBufferContentAndReset(buf); > > - > > - VIR_FREE(str); > > + if (buf) > > + virBufferSetError(buf, 0); > > You saved 1 call to memset. I also don't see how we can break > virBufferContentAndReset in a way that we couldn't mess up virBufferSetError in > the same way. Additionally, seeing a call to virBufferSetError in a free helper > looks suspicious (even if it has a '0' as argument). If anything, I'd rename > virBuggerFreeAndReset to virBufferClean. Thing is, if 1/3 wasn't merged, then the test I'm introducing there would leak memory. But not with this patch merged. So from long term perspective, if there is ever some similar bug we won't leak any memory. Yeah, naming is hard.