Re: [libvirt] [PATCH 1/3] virbuffer: Don't leak memory in virBufferAddBuffer
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(a)redhat.com>
whether or not you switch to VIR_AUTOCLEAN() - but please do :)
--
Andrea Bolognani / Red Hat / Virtualization