Re: [Qemu-devel] [PATCH v15 10/23] qmp-input: Require struct push to visit members of top dict

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Don't embed the root of the visit into the stack of current
> containers being visited.  That way, we no longer get confused
> on whether the first visit of a dictionary is to the dictionary
> itself or to one of the members of the dictionary, based on
> whether the caller passed name=NULL; and makes the QMP Input
> visitor like other visitors where the value of 'name' is now
> ignored on the root visit.  (We may someday want to revisit
> the rules on what 'name' should be on a top-level visit,
> rather than just ignoring it; but that would be the topic of
> another patch).
>
> An audit of all qmp_input_visitor_new() call sites shows that
> there were only two places where callers had previously been
> visiting to a QDict with a non-NULL name to bypass a call to
> visit_start_struct(), and those were fixed in prior patches.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v15: hoist earlier in series, improve variable naming
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qapi/qmp-input-visitor.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index cae6387..6409a83 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -35,9 +35,11 @@ struct QmpInputVisitor
>  {
>      Visitor visitor;
>
> -    /* Stack of objects being visited.  stack[0] is root of visit,
> -     * stack[1..] records the nesting of start_struct()/end_struct()
> -     * and start_list()/end_list() pairs. */
> +    /* Root of visit at visitor creation. */
> +    QObject *root;
> +
> +    /* Stack of objects being visited (all entries will be either
> +     * QDict or QList). */
>      StackObject stack[QIV_STACK_SIZE];
>      int nb_stack;
>
> @@ -54,33 +56,34 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>                                       const char *name,
>                                       bool consume)
>  {
> -    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
> -    QObject *qobj = tos->obj;
> -    QObject *ret;
> +    StackObject *tos;
> +    QObject *qobj;
> +    QObject *ret = NULL;

Dead initializer?

>
> +    if (!qiv->nb_stack) {
> +        /* Starting at root, name is ignored. */
> +        return qiv->root;
> +    }
> +
> +    /* We are in a container; find the next element. */
> +    tos = &qiv->stack[qiv->nb_stack - 1];
> +    qobj = tos->obj;
>      assert(qobj);
>
> -    /* If we have a name, and we're in a dictionary, then return that
> -     * value. */
> -    if (name && qobject_type(qobj) == QTYPE_QDICT) {
> +    if (qobject_type(qobj) == QTYPE_QDICT) {
> +        assert(name);
>          ret = qdict_get(qobject_to_qdict(qobj), name);
>          if (tos->h && consume && ret) {
>              bool removed = g_hash_table_remove(tos->h, name);
>              assert(removed);
>          }
> -        return ret;
> -    }
> -
> -    /* If we are in the middle of a list, then return the next element
> -     * of the list. */
> -    if (tos->entry) {
> +    } else {
>          assert(qobject_type(qobj) == QTYPE_QLIST);
> -        return qlist_entry_obj(tos->entry);
> +        assert(!name);
> +        ret = qlist_entry_obj(tos->entry);
>      }
>
> -    /* Otherwise, we are at the root of the visit or the start of a
> -     * list, and return the object as-is. */
> -    return qobj;
> +    return ret;
>  }
>
>  static void qdict_add_key(const char *key, QObject *obj, void *opaque)
> @@ -355,7 +358,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
>
>  void qmp_input_visitor_cleanup(QmpInputVisitor *v)
>  {
> -    qobject_decref(v->stack[0].obj);
> +    qobject_decref(v->root);
>      g_free(v);
>  }
>
> @@ -381,7 +384,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool 
> strict)
>      v->visitor.optional = qmp_input_optional;
>      v->strict = strict;
>
> -    qmp_input_push(v, obj, NULL);
> +    v->root = obj;
>      qobject_incref(obj);
>
>      return v;