[libvirt] [PATCH 1/3] virbuffer: Don't leak memory in virBufferAddBuffer

[Andrea Bolognani]

On Fri, 2019-05-03 at 11:57 +0200, Michal Privoznik wrote:
> On 5/3/19 11:18 AM, Andrea Bolognani wrote:
> > On Thu, 2019-04-18 at 14:11 +0200, Michal Privoznik wrote:
> > > +    if (virBufferCurrentContent(&buf1) ||
> > > +        !virBufferCurrentContent(&buf2)) {
> > > +        VIR_TEST_DEBUG("Unexpected buffer content");
> > > +        goto cleanup;
> > > +    }
> > 
> > And here you check both buffers. This is the part I don't get: since
> > virBufferCurrentContent() returns NULL for errored buffers, shouldn't
> > we get NULL in both cases here? And should we not get that result
> > both with and *without* your patch? We were already setting the error
> > flag for buf1 after all...
> > 
> > I tried reverting the changes to virBufferAddBuffer() made in this
> > commit, and 'make check' still passes for me, which matches my
> > observation above but certainly not my original expectations.
> 
> Firstly, it's not about passing the test but about leaking memory. Try 
> running under valgrind.

Okay, this is the first bit that I was missing, though I suspected
something along those lines. FWIW it would have been more obvious,
at least to me, what was going on if you had introduced the test
first, in a separate commit, and then fixed the function, but the
way you're doing it is just fine. I blame Friday, and not having
had much coffee in the morning :)

> Secondly, virBufferAddBuffer() resets the other buffer, so even if it 
> had an error it no longer has it after virBufferAddBuffer() is called. 
> And no error means virBufferCurrentContent() returns an empty string 
> (because the buffer has no content).

This is the second, and most important, bit I was missing: the fact
that resetting the buffer also resets the error flag. It was right
in front of my eyes, but I was not seeing it... See above again for
remarks about weekdays and beverages.

> Hopefully this clears your concerns.

It sure does! Thanks for taking the time to explain in detail.

  Reviewed-by: Andrea Bolognani <abologna at redhat.com>

whether or not you switch to VIR_AUTOCLEAN() - but please do :)

-- 
Andrea Bolognani / Red Hat / Virtualization