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

[Erik Skultety]

On Fri, May 03, 2019 at 11:18:09AM +0200, Michal Privoznik wrote:
> 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 at redhat.com>
> > > ---
> > >   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.

Reviewed-by: Erik Skultety <eskultet at redhat.com>