Eric Blake <ebl...@redhat.com> writes:
> On 09/11/2015 02:43 PM, Eric Blake wrote:
>
>>> +++ b/tests/test-qmp-output-visitor.c
>>> @@ -485,7 +485,7 @@ static void
>>> test_visitor_out_empty(TestOutputVisitorData *data,
>>> QObject *arg;
>>>
>>> arg = qmp_output_get_qobject(data->qov);
>>> - g_assert(!arg);
>>> + g_assert(qobject_type(arg) == QTYPE_QNULL);
>>> }
>>
>> Missing
>> qobject_decref(arg);
>>
>> Ultimately, the global qnull_ starts life with refcnt 1, and after this
>> test should be back to 1. But it is coming back as 3, so even with a
>> qobject_decref, that would get it back down to 2. So we are leaking a
>> reference to qnull somewhere.
Relatively harmless, because the qnull_ singleton is static. Worth
fixing anyway, of course.
>> I'm still investigating, and may be able to find the patch
>
> Squash this in, and you can have:
> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> diff --git i/qapi/qmp-output-visitor.c w/qapi/qmp-output-visitor.c
> index 2d6083e..b96849e 100644
> --- i/qapi/qmp-output-visitor.c
> +++ w/qapi/qmp-output-visitor.c
> @@ -66,11 +66,7 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
> {
> QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
>
> - if (!e) {
> - return qnull();
> - }
> -
> - return e->value;
> + return e ? e->value : NULL;
> }
>
> static QObject *qmp_output_last(QmpOutputVisitor *qov)
> @@ -190,6 +186,8 @@ QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
> QObject *obj = qmp_output_first(qov);
> if (obj) {
> qobject_incref(obj);
> + } else {
> + obj = qnull();
> }
> return obj;
> }
The visitor code is disgustingly ambiguous / confused / confusing about
NULL. The whole thing feels like it was hacked up in a rush. We've
been paying interest for it not having a written contract ever since.
I put the qnull() in qmp_output_first() to avoid (QObject *)0 entirely.
Also because that's where I found the FIXME :)
You lift it into one of two callers. Impact:
* qmp_output_get_qobject()
- master: return NULL when !e || !e->value
- my patch: return qnull() when !e
return NULL when !e->value
- your patch: return qnull() when !e || !e->value
* qmp_output_visitor_cleanup()
- master: root = NULL when when !e || !e->value
- my patch: root = qnull() when !e
root = NULL when !e->value
- your patch: root = NULL when when !e || !e->value
where e = QTAILQ_LAST(&qov->stack, QStack)
Questions:
* How can e become NULL?
The only place that pushes onto &qov->stack appears to be
qmp_output_push_obj(). Can obviously push e with !e->value, but can't
push null e.
* What about qmp_output_last()?
Why does qmp_output_first() check for !e, but not qmp_output_last()?
My patch makes them less symmetric (bad smell). Yours makes them more
symmetric, but not quite.
* How does this contraption work?
> diff --git i/tests/test-qmp-output-visitor.c
> w/tests/test-qmp-output-visitor.c
> index 256bffd..8bd48f4 100644
> --- i/tests/test-qmp-output-visitor.c
> +++ w/tests/test-qmp-output-visitor.c
> @@ -486,6 +486,9 @@ static void
> test_visitor_out_empty(TestOutputVisitorData *data,
>
> arg = qmp_output_get_qobject(data->qov);
> g_assert(qobject_type(arg) == QTYPE_QNULL);
> + /* Check that qnull reference counting is sane */
> + g_assert(arg->refcnt == 2);
> + qobject_decref(arg);
> }
>
> static void init_native_list(UserDefNativeListUnion *cvalue)
