[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnu
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL |
Date: |
Sat, 12 Sep 2015 10:10:09 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
>
> 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)
- [Qemu-devel] [PATCH v6 11/26] qapi-event: Convert to QAPISchemaVisitor, fixing data with base, (continued)
- [Qemu-devel] [PATCH v6 11/26] qapi-event: Convert to QAPISchemaVisitor, fixing data with base, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 17/26] qapi: De-duplicate parameter list generation, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 13/26] qapi: Clean up after recent conversions to QAPISchemaVisitor, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 01/26] qapi: Rename class QAPISchema to QAPISchemaParser, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 23/26] qapi-schema: Fix up misleading specification of netdev_add, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 19/26] qapi: Improve built-in type documentation, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 18/26] qapi-commands: De-duplicate output marshaling functions, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 15/26] qapi-commands: Rearrange code, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 26/26] qapi-introspect: Hide type names, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 24/26] qapi: Pseudo-type '**' is now unused, drop it, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 16/26] qapi: Rename qmp_marshal_input_FOO() to qmp_marshal_FOO(), Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 21/26] qapi: Introduce a first class 'any' type, Markus Armbruster, 2015/09/11