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@redhat.com> whether or not you switch to VIR_AUTOCLEAN() - but please do :) -- Andrea Bolognani / Red Hat / Virtualization