Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL

[Eric Blake]

On 09/12/2015 08:11 AM, Markus Armbruster wrote:

>> Indeed. Without reading further, we're both shooting in the dark for
>> something that makes tests pass, but without being a clean interface.
>>
>> How about this: go ahead with your series as proposed, with the squash
>> hunk to tests/ to avoid the leak in the testsuite, but leaving the leak
>> in qmp_output_get_object(), and we address the leak in a followup patch.

I've given a couple more responses with my analysis, but up to you how
much you want to take now or defer to later.

> 
> I'll add a FIXME explaining the reference counting bug, and the wider
> problem.
> 
> What exactly do you want changed in tests?

Definitely add the qobject_decref(arg). But the g_assert(refcnt...)
should not be added unless we go with a solution that doesn't leak, and
instead should be replaced with a FIXME, if you don't want my followup
mails now.


>> The followup patch(es) will then have to include some contract
>> documentation, so that we track what we learned while investigating the
>> code, and so that the next reader has more than just code to start from.
> 
> It's time to retrofit visitors with a proper contract.
> 
> Retrofitting a contract is much harder than starting with one, but we
> got to play the hand we've been dealt.

I've already started that work in some of my pending patches:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02597.html

but it could indeed use further documentation in each visitor
implementation.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature