Eric Blake <ebl...@redhat.com> writes:
> Commit 6c2f9a15 ensured that we would not return NULL when the
> caller used an output visitor but had nothing to visit. But
> in doing so, it added a FIXME about a reference count leak
> that could abort qemu in the (unlikely) case of SIZE_MAX such
> visits (more plausible on 32-bit).
The commit message says "We'll want to fix it for real before the
release." Is this patch for 2.5? For what it's worth, it applies to
master.
> This fixes things by documenting the internal contracts, and
> explaining why the internal function can return NULL and only
> the public facing interface needs to worry about qnull(),
> thus avoiding over-referencing the qnull_ global object.
>
> It does not, however, fix the stupidity of the stack mixing
> up two separate pieces of information; add a FIXME to explain
> that issue.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v6: no change
> ---
> qapi/qmp-output-visitor.c | 30 ++++++++++++++++++++++++++++--
> tests/test-qmp-output-visitor.c | 2 ++
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index e66ab3c..9d0f9d1 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -29,6 +29,12 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack;
> struct QmpOutputVisitor
> {
> Visitor visitor;
> + /* FIXME: we are abusing stack to hold two separate pieces of
> + * information: the current root object, and the stack of objects
> + * still being built. Worse, our behavior is inconsistent:
> + * visiting two top-level scalars in a row discards the first in
> + * favor of the second, but visiting two top-level objects in a
> + * row tries to append the second object into the first. */
Uhh, I'll simply take your word for it.
> QStack stack;
> };
>
> @@ -41,10 +47,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
> return container_of(v, QmpOutputVisitor, visitor);
> }
>
> +/* Push @value onto the stack of current QObjects being built */
> static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
> {
> QStackEntry *e = g_malloc0(sizeof(*e));
>
> + assert(value);
> e->value = value;
Keep in mind for later: we never push a null value on the stack, and
there is no other way to grow the stack. Therefore, the stack holds
only non-null values.
> if (qobject_type(e->value) == QTYPE_QLIST) {
> e->is_list_head = true;
> @@ -52,16 +60,20 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov,
> QObject *value)
> QTAILQ_INSERT_HEAD(&qov->stack, e, node);
> }
>
> +/* Grab and remove the most recent QObject from the stack */
> static QObject *qmp_output_pop(QmpOutputVisitor *qov)
> {
> QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> QObject *value;
> +
> + assert(e);
I figure this assert the stack isn't empty. Correct?
> QTAILQ_REMOVE(&qov->stack, e, node);
> value = e->value;
> g_free(e);
> return value;
> }
>
> +/* Grab the root QObject, if any, in preparation to empty the stack */
> static QObject *qmp_output_first(QmpOutputVisitor *qov)
> {
> QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> @@ -72,24 +84,32 @@ static QObject *qmp_output_first(QmpOutputVisitor *qov)
/*
* FIXME Wrong, because qmp_output_get_qobject() will increment
* the refcnt *again*. We need to think through how visitors
> * handle null.
> */
Sure you don't want to drop the FIXME?
> if (!e) {
I figure this means the stack is empty. Now I wish I understood how we
use the stack.
> - return qnull();
> + /* No root */
> + return NULL;
> }
> -
> + assert(e->value);
Safe because the stack holds only non-null values.
> return e->value;
> }
We return null exactly when the stack is empty (whatever that may mean).
>
> +/* Grab the most recent QObject from the stack, which must exist */
> static QObject *qmp_output_last(QmpOutputVisitor *qov)
> {
> QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> +
> + assert(e);
> return e->value;
> }
Can't return null, because the stack holds only non-null values.
assert(e && e->value) to match qmp_output_first()?
>
> +/* Add @value to the current QObject being built.
> + * If the stack is visiting a dictionary or list, @value is now owned
> + * by that container. Otherwise, @value is now the root. */
> static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
> QObject *value)
> {
> QObject *cur;
>
> if (QTAILQ_EMPTY(&qov->stack)) {
> + /* Stack was empty, track this object as root */
> qmp_output_push_obj(qov, value);
> return;
> }
> @@ -98,13 +118,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov,
> const char *name,
>
> switch (qobject_type(cur)) {
> case QTYPE_QDICT:
> + assert(name);
Okay, but it really only converts one kind of crash into another one.
> qdict_put_obj(qobject_to_qdict(cur), name, value);
> break;
> case QTYPE_QLIST:
> qlist_append_obj(qobject_to_qlist(cur), value);
> break;
> default:
> + /* The previous root was a scalar, replace it with a new root */
> qobject_decref(qmp_output_pop(qov));
> + assert(QTAILQ_EMPTY(&qov->stack));
> qmp_output_push_obj(qov, value);
> break;
> }
Hand me the bucket.
> @@ -205,11 +228,14 @@ static void qmp_output_type_any(Visitor *v, QObject
> **obj, const char *name,
> qmp_output_add_obj(qov, name, *obj);
> }
>
> +/* Finish building, and return the root object. Will not be NULL. */
> QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
> {
> QObject *obj = qmp_output_first(qov);
> if (obj) {
Non-empty stack.
> qobject_incref(obj);
> + } else {
Empty stack.
> + obj = qnull();
> }
> return obj;
> }
Thanks for your comments. They help a lot with understanding the code,
and they also demonstrate that it's muddled crap %-}
So, how does this contraption work?
A visitor cab encounter NULL only when it visits pointers (d'oh!).
Searching qapi-visit-core.c for **obj finds start_struct(),
start_implicit_struct(), type_str(), type_any().
As far as I can tell, start_implicit_struct() is for unboxed structs, so
NULL must not happen there. This visitor doesn't implement it anyway.
qmp_output_type_str() substitutes "" for NULL. Ooookay.
qmp_output_type_any() crashes on NULL. Can this happen?
qmp_output_start_struct() ignores its obj argument. My best guess is
that this substitutes an empty dictionary for NULL.
Okay, I give up: how do we get an empty stack and return qnull()?
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 3078442..8e6fc33 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -461,6 +461,8 @@ 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);
> }
