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@xxxxxxxxxx>
whether or not you switch to VIR_AUTOCLEAN() - but please do :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
