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

[Markus Armbruster]

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)