Re: [Qemu-devel] [PATCH v14 17/19] 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

I got confused pretty much every time I looked at this code...

> on whether the first visit of a dictionary is to the dictionary
> itself or to one of the members of the dictionary, and we no
> longer have to require the root visit to pass name=NULL.
>
> An audit of all qmp_input_visitor_new* call sites shows that
> the only places where the visit starts directly on a QDict,
> but where the first visit_type_* within the visit had passed
> a non-NULL name, were fixed in the previous two places to
> properly push into the object with visit_start_struct().
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: no change
> v13: no change
> v12: new patch
> ---
>  include/qapi/visitor.h           |  3 +--
>  include/qapi/qmp-input-visitor.h |  8 ------
>  qapi/qmp-input-visitor.c         | 54 
> ++++++++++++++++++++++------------------
>  3 files changed, 31 insertions(+), 34 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4c29722..e1213a3 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -47,8 +47,7 @@
>   *
>   * The @name parameter of visit_type_FOO() describes the relation
>   * between this QAPI value and its parent container.  When visiting
> - * the root of a tree, @name is usually ignored (although some
> - * visitors require it to be NULL); when visiting a member of an
> + * the root of a tree, @name is ignored; when visiting a member of an
>   * object, @name is the key associated with the value; and when
>   * visiting a member of a list, @name is NULL.
>   *

Should we require a root visit's name to be null?  Not in this patch.

> diff --git a/include/qapi/qmp-input-visitor.h 
> b/include/qapi/qmp-input-visitor.h
> index d75ff98..3ed499c 100644
> --- a/include/qapi/qmp-input-visitor.h
> +++ b/include/qapi/qmp-input-visitor.h
> @@ -19,14 +19,6 @@
>
>  typedef struct QmpInputVisitor QmpInputVisitor;
>
> -/*
> - * FIXME: When visiting a QDict, passing a non-NULL @name for the
> - * first visit_type_FOO() when the root is a QDict will find that
> - * particular key within the QDict.  In the future, the contract may
> - * be tightened to require visit_start_struct() with ignored @name as
> - * the first visit; in the meantime, the first visit is safest when
> - * using NULL for @name.
> - */
>  QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
>  QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index a94cfa9..16f2f5a 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -39,9 +39,11 @@ struct QmpInputVisitor
>  {
>      Visitor visitor;
>
> -    /* Stack of objects being visited.  stack[0] is root of visit,
> -     * stack[1] and below correspond to visit_start_struct (nested
> -     * QDict) and visit_start_list (nested QList).  */
> +    /* 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;
>
> @@ -58,36 +60,40 @@ 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;
> +    StackObject *tos;
> +    QObject *qobj;
>
> +    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);
>          qobj = qdict_get(qobject_to_qdict(qobj), name);
>          if (tos->h && consume && qobj) {
>              bool removed = g_hash_table_remove(tos->h, name);
>              assert(removed);
>          }
> -        return qobj;
> -    }
> -
> -    /* 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);
> -        assert(!tos->first);
> -        qobj = qlist_entry_obj(tos->entry);
> -        if (consume) {
> -            tos->entry = qlist_next(tos->entry);
> +        /* FIXME: assertion needs adjustment if we fix visit-core
> +         * to pass "name.0" style name during lists.  */

My remarks to the same comment in PATCH 13 apply.

Additionally, if we want a comment on name here, is this the correct
patch to add it?

> +        assert(!name);
> +
> +        if (tos->entry) {
> +            assert(!tos->first);
> +            qobj = qlist_entry_obj(tos->entry);
> +            if (consume) {
> +                tos->entry = qlist_next(tos->entry);
> +            }
>          }
> -        return qobj;
>      }
> -
> -    /* Otherwise, we are at the root of the visit or the start of a
> -     * list, and return the object as-is.  */
>      return qobj;
>  }
>

qobj has two roles in this function.  First, it's tos->obj.  Then it
morphs into the function result, which is either the member of tos->obj
named by name, the proper tail of tos->obj pointed to by tos->entry, or
tos->obj itself.  I'd prefer to keep them separate, perhaps like this:

       /* We are in a container; find the next element */
       tos = &qiv->stack[qiv->nb_stack - 1];
       assert(tos->qobj);

       if (qobject_type(tos->qobj) == QTYPE_QDICT) {
           assert(name);
           qobj = qdict_get(qobject_to_qdict(tos->qobj), name);
           if (tos->h && consume && qobj) {
               bool removed = g_hash_table_remove(tos->h, name);
               assert(removed);
           }
       } else {
           assert(qobject_type(tos->qobj) == QTYPE_QLIST);
           assert(!name);

           if (tos->entry) {
               assert(!tos->first);
               qobj = qlist_entry_obj(tos->entry);
               if (consume) {
                   tos->entry = qlist_next(tos->entry);
               }
           } else {
               qobj = tos->qobj;
           }
       }
       return qobj;

Having a second variable to hold tos->qobj would fine, too.

Something else that's bugging me: we step tos->entry here only for the
"not first" case.  For the "first" case, we do it in qmp_input_push().
Could we clean that up?  Not necessarily in this patch or even series.

> @@ -373,7 +379,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);
>  }
>
> @@ -400,7 +406,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.type_null = qmp_input_type_null;
>      v->visitor.optional = qmp_input_optional;
>
> -    qmp_input_push(v, obj, NULL, NULL);
> +    v->root = obj;
>      qobject_incref(obj);
>
>      return v;