Re: [Qemu-devel] [PATCH v9 20/37] qmp: Don't abuse stack to track qmp-output root

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> The previous commit documented an inconsistency in how we are
> using the stack of qmp-output-visitor.  Normally, pushing a
> single top-level object puts the object on the stack twice:
> once as the root, and once as the current container being
> appended to; but popping that struct only pops once.  However,
> qmp_ouput_add() was trying to either set up the added object
> as the new root (works if you parse two top-level scalars in a
> row: the second replaces the first as the root) or as a member
> of the current container (works as long as you have an open
> container on the stack; but if you have popped the first
> top-level container, it then resolves to the root and still
> tries to add into that existing container).
>
> Fix the stupidity by not tracking two separate things in the
> stack.  Drop the now-useless qmp_output_first() while at it.
>
> Saved for a later patch: we still are rather sloppy in that
> qmp_output_get_object() can be called in the middle of a parse,
> rather than requiring that a visit is complete.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: rebase to earlier changes
> v7: retitle; rebase to earlier changes, drop qmp_output_first()
> v6: no change
> ---
>  qapi/qmp-output-visitor.c | 79 ++++++++++++++++-------------------------------
>  1 file changed, 26 insertions(+), 53 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 316f4e4..df22999 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -29,16 +29,8 @@ 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 in slot 0, and the stack
> -     * of N objects still being built in slots 1 through N (for N+1
> -     * slots in use).  Worse, our behavior is inconsistent:
> -     * qmp_output_add_obj() 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 (since the first object was placed in the stack
> -     * in both slot 0 and 1, but only popped from slot 1).  */
> -    QStack stack;
> +    QStack stack; /* Stack of containers that haven't yet been finished */
> +    QObject *root; /* Root of the output visit */
>  };
>
>  #define qmp_output_add(qov, name, value) \
> @@ -55,6 +47,7 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
>  {
>      QStackEntry *e = g_malloc0(sizeof(*e));
>
> +    assert(qov->root);

This is safe because every call is preceded by qmp_output_add(), which
creates the root if we don't have one, yet.

>      assert(value);
>      e->value = value;
>      if (qobject_type(e->value) == QTYPE_QLIST) {
> @@ -76,26 +69,12 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>      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);
> -
> -    if (!e) {
> -        /* No root */
> -        return NULL;
> -    }
> -    assert(e->value);
> -    return e->value;
> -}
> -
> -/* Grab the most recent QObject from the stack, which must exist */
> +/* Grab the most recent QObject from the stack, if any */
>  static QObject *qmp_output_last(QmpOutputVisitor *qov)
>  {
>      QStackEntry *e = QTAILQ_FIRST(&qov->stack);
>
> -    assert(e && e->value);
> -    return e->value;
> +    return e ? e->value : NULL;
>  }

Okay, because...

>
>  /* Add @value to the current QObject being built.
> @@ -106,29 +85,25 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>  {
>      QObject *cur;
>
> -    if (QTAILQ_EMPTY(&qov->stack)) {
> -        /* Stack was empty, track this object as root */
> -        qmp_output_push_obj(qov, value);
> -        return;
> -    }
> -
>      cur = qmp_output_last(qov);

... this is the only caller, and...

>
> -    switch (qobject_type(cur)) {
> -    case QTYPE_QDICT:
> -        assert(name);
> -        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 */
> -        /* FIXME this is abusing the stack; see comment above */
> -        qobject_decref(qmp_output_pop(qov));
> -        assert(QTAILQ_EMPTY(&qov->stack));
> -        qmp_output_push_obj(qov, value);
> -        break;
> +    if (!cur) {

... you add the null check.

> +        /* FIXME we should require the user to reset the visitor, rather
> +         * than throwing away the previous root */
> +        qobject_decref(qov->root);
> +        qov->root = value;
> +    } else {
> +        switch (qobject_type(cur)) {
> +        case QTYPE_QDICT:
> +            assert(name);
> +            qdict_put_obj(qobject_to_qdict(cur), name, value);
> +            break;
> +        case QTYPE_QLIST:
> +            qlist_append_obj(qobject_to_qlist(cur), value);
> +            break;
> +        default:
> +            g_assert_not_reached();

We usually just abort().

> +        }
>      }
>  }

Let's see how this works.

We have a root and a stack of containers.

qmp_output_visitor_new() creates no root and an empty stack.

qmp_output_visitor_cleanup() empties the stack and throws away the root.

Visitors of scalars call qmp_output_add_obj().  Visitors of containers
(object a.k.a. struct or array a.k.a. list) do the same, and
additionally call qmp_output_push_obj(), in their start method.  They
call qmp_output_pop() in their end method.  What does this do?

If the stack is empty (!cur), visiting a value replaces the root, if
any.  Begs the question how can we have a root, but let's ignore that
for now.

If the stack isn't empty, we add to the container on the top of the
stack.

If we're starting to visit a container, we push it onto the stack.  It
remains there until we end the container's visit.

Okay, this works.

After an outermost visit completes, the stack is empty, but the root
lingers.  It gets thrown away only when you reuse the visitor for
another visit (do we do that?), or you destroy the visitor.  Letting the
root linger that way isn't nice, and that's what your FIXME points out.

If you abort a visit half-way through, the stack may be non-empty.  You
can't just start another visit then.  You have to destroy the visitor
and create a new one.

If we required a "reset" between visits, as your FIXME suggests, the
difference between completed and aborted visits can be removed.

>
> @@ -230,7 +205,9 @@ static void qmp_output_type_any(Visitor *v, const char *name, QObject **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);
> +    /* FIXME: we should require that a visit occurred, and that it is
> +     * complete (no starts without a matching end) */

I agree the visit must complete before you can retrieve the value.

I think there are two sane ways to recover from errors:

1. Require the client to empty the stack by calling the necessary end
   methods.

2. Allow the client to reset or destroy the visitor without calling end
   methods.

*This* visitor would be fine with either.  I guess the others would be
fine, too.  So it's a question of interface design.

I'm currently leaning towards 2, because "you must do A, B and C before
you can destroy this object" would be weird.  What do you think?

> +    QObject *obj = qov->root;
>      if (obj) {
>          qobject_incref(obj);
>      } else {
> @@ -248,16 +225,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
>  {
>      QStackEntry *e, *tmp;
>
> -    /* The bottom QStackEntry, if any, owns the root QObject. See the
> -     * qmp_output_push_obj() invocations in qmp_output_add_obj(). */
> -    QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v);
> -

If we require end methods to be called, the stack must be empty here,
rendering the following loop useless.

>      QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) {
>          QTAILQ_REMOVE(&v->stack, e, node);
>          g_free(e);
>      }
>
> -    qobject_decref(root);
> +    qobject_decref(v->root);
>      g_free(v);
>  }