Re: [Qemu-devel] [PATCH v14 05/19] qmp-input: Clean up stack handling

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Management of the top of stack was a bit verbose; creating a
> temporary variable and adding some comments makes the existing
> code more legible before the next few patches improve things.
> No semantic changes other than asserting that we are always
> visiting a QObject, and not a NULL value.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qapi/qmp-input-visitor.c | 52 
> ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 77cce8b..7ba6d3d 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -25,16 +25,26 @@
>
>  typedef struct StackObject
>  {
> -    QObject *obj;
> +    QObject *obj; /* Object being visited */
> +
> +    /* If obj is list: NULL if list is at head, otherwise tail of list
> +     * still needing visits */

"still needing visits?"

>      const QListEntry *entry;
> -    GHashTable *h;
> +
> +    GHashTable *h; /* If obj is dict: remaining keys needing visits */

Ah, now I get it.  Swap the two?

>  } StackObject;

The mixture of block comments and comments to the right is a bit
awkward.  What about:

   typedef struct StackObject {
       QObject *obj; /* Object being visited */

       GHashTable *h;              /* if obj is dict: unvisited keys */
       const QListEntry *entry;    /* if obj is list: unvisited tail */
   } StackObject;

>
>  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).  */

I guess what you want to say is stack[1..] record the nesting of
start_struct() ... end_struct() and start_list() ... end_list() pairs.

Comment gets rewritten in PATCH 17, no need to worry too much about it.

>      StackObject stack[QIV_STACK_SIZE];
>      int nb_stack;
> +
> +    /* True to track whether all keys in QDict have been parsed.  */
>      bool strict;

I think @strict switches on rejection of unexpected dictionary keys.
See qmp_input_pop() below.

I dislike the fact that we have two input visitors, and the one with the
obvious name ignores certain errors.  I don't doubt that it has its
uses, but reporting errors should be the default, and ignoring them
should be a conscious decision.  Anyway, not this patch's problem.

>  };
>
> @@ -47,19 +57,29 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>                                       const char *name,
>                                       bool consume)
>  {
> -    QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
> +    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
> +    QObject *qobj = tos->obj;
>
> -    if (qobj) {
> -        if (name && qobject_type(qobj) == QTYPE_QDICT) {
> -            if (qiv->stack[qiv->nb_stack - 1].h && consume) {
> -                g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
> -            }
> -            return qdict_get(qobject_to_qdict(qobj), name);
> -        } else if (qiv->stack[qiv->nb_stack - 1].entry) {
> -            return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> +    assert(qobj);
> +
> +    /* If we have a name, and we're in a dictionary, then return that
> +     * value. */

Can we be in a dictionary and not have a name?

Either one...

> +    if (name && qobject_type(qobj) == QTYPE_QDICT) {
> +        if (tos->h && consume) {
> +            g_hash_table_remove(tos->h, name);
>          }
> +        return qdict_get(qobject_to_qdict(qobj), name);
>      }
>
> +    /* If we are in the middle of a list, then return the next element
> +     * of the list.  */

... or two spaces between end of sentence and */.  I like the
old-fashioned double space between sentences myself, but not before */.
Regardless of what I like, we should try to be consistent.

I got used to winged comments, where this issue is moot.

> +    if (tos->entry) {
> +        assert(qobject_type(qobj) == QTYPE_QLIST);
> +        return 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;
>  }
>
> @@ -72,20 +92,22 @@ static void qdict_add_key(const char *key, QObject *obj, 
> void *opaque)
>  static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>  {
>      GHashTable *h;
> +    StackObject *tos = &qiv->stack[qiv->nb_stack];
>
> +    assert(obj);
>      if (qiv->nb_stack >= QIV_STACK_SIZE) {
>          error_setg(errp, "An internal buffer overran");
>          return;
>      }
>
> -    qiv->stack[qiv->nb_stack].obj = obj;
> -    qiv->stack[qiv->nb_stack].entry = NULL;
> -    qiv->stack[qiv->nb_stack].h = NULL;
> +    tos->obj = obj;
> +    tos->entry = NULL;
> +    tos->h = NULL;
>
>      if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
>          h = g_hash_table_new(g_str_hash, g_str_equal);
>          qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
> -        qiv->stack[qiv->nb_stack].h = h;
> +        tos->h = h;
>      }
>
>      qiv->nb_stack++;
   }


   static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
   {
       assert(qiv->nb_stack > 0);

       if (qiv->strict) {
           GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
           if (top_ht) {
               GHashTableIter iter;
               const char *key;

               g_hash_table_iter_init(&iter, top_ht);
               if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
                   error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);

This looks wrong.  If we have more than one extra members, the second
call error_setg() will fail an assertion in error_setv(), unless errp is
null.

               }
               g_hash_table_unref(top_ht);
           }
       }

       qiv->nb_stack--;
   }