Re: [Qemu-devel] [PATCH v15 11/23] qmp-input: Refactor when list is advanced

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> In the QMP input visitor, visiting a list traverses two objects:
> the QAPI GenericList of the caller (which gets advanced in
> visit_next_list() regardless of this patch), and the QList input
> that we are converting to QAPI.  For consistency with QDict
> visits, we want to consume elements from the input QList during
> the visit_type_FOO() for the list element; that is, we want ALL
> the code for consuming an input to live in qmp_input_get_object(),
> rather than having it split according to whether we are visiting
> a dict or a list.  Making qmp_input_get_object() the common point
> of consumption will make it easier for a later patch to refactor
> visit_start_list() to cover the GenericList * head of a QAPI list,
> and in turn will get rid of the 'first' flag (which lived in
> qmp_input_next_list() pre-patch, and is hoisted to StackObject
> by this patch).
>
> This patch is therefore shifting the post-condition use of
> 'entry' from:
>
>         start_list next_list type_ELT ... next_list type_ELT end_list
>  entry  NULL       1st elt   1st elt      last elt  last elt gone
>
> where type_ELT() returns (entry ? entry : 1st elt) and next_list() steps
> entry
>
> to this usage:
>
>         start_list next_list type_ELT ... next_list type_ELT end_list
>  entry  1st elt    1nd elt   2nd elt      last elt  NULL     gone
>
> where type_ELT() steps entry and returns the old entry, and next_list()
> leaves entry alone.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v15: rebase, improve commit message, hoist root handling earlier
> so that this patch is more pinpointed
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qapi/qmp-input-visitor.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 6409a83..eb77adc 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -29,6 +29,7 @@ typedef struct StackObject
>
>      GHashTable *h;           /* If obj is dict: unvisited keys */
>      const QListEntry *entry; /* If obj is list: unvisited tail */
> +    bool first;              /* If obj is list: will next_list() visit head 
> */

I like documenting bools with the yes/no question they answer.
Questions end with a question mark, though :)

Perhaps: /* If obj is list: next_list() not yet called? */

Since this will go away soon, feel free not to change anything here.

>  } StackObject;
>
>  struct QmpInputVisitor
> @@ -80,7 +81,11 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>      } else {
>          assert(qobject_type(qobj) == QTYPE_QLIST);
>          assert(!name);
> +        assert(!tos->first);
>          ret = qlist_entry_obj(tos->entry);
> +        if (consume) {
> +            tos->entry = qlist_next(tos->entry);
> +        }
>      }
>
>      return ret;
> @@ -104,13 +109,16 @@ static void qmp_input_push(QmpInputVisitor *qiv, 
> QObject *obj, Error **errp)
>      }
>
>      tos->obj = obj;
> -    tos->entry = NULL;
> -    tos->h = NULL;
> +    assert(!tos->h);
> +    assert(!tos->entry);
>
>      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);
>          tos->h = h;
> +    } else if (qobject_type(obj) == QTYPE_QLIST) {
> +        tos->entry = qlist_first(qobject_to_qlist(obj));
> +        tos->first = true;
>      }
>
>      qiv->nb_stack++;
> @@ -119,10 +127,11 @@ static void qmp_input_push(QmpInputVisitor *qiv, 
> QObject *obj, Error **errp)
>
>  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>  {
> +    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
>      assert(qiv->nb_stack > 0);
>
>      if (qiv->strict) {
> -        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
> +        GHashTable * const top_ht = tos->h;

Let's clean this up to *const while we're touching it.  Can do on
commit.

>          if (top_ht) {
>              GHashTableIter iter;
>              const char *key;
> @@ -133,6 +142,7 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error 
> **errp)
>              }
>              g_hash_table_unref(top_ht);
>          }
> +        tos->h = NULL;
>      }
>
>      qiv->nb_stack--;
> @@ -192,23 +202,15 @@ static GenericList *qmp_input_next_list(Visitor *v, 
> GenericList **list,
>      QmpInputVisitor *qiv = to_qiv(v);
>      GenericList *entry;
>      StackObject *so = &qiv->stack[qiv->nb_stack - 1];
> -    bool first;
>
> -    if (so->entry == NULL) {
> -        so->entry = qlist_first(qobject_to_qlist(so->obj));
> -        first = true;
> -    } else {
> -        so->entry = qlist_next(so->entry);
> -        first = false;
> -    }
> -
> -    if (so->entry == NULL) {
> +    if (!so->entry) {
>          return NULL;
>      }
>
>      entry = g_malloc0(size);
> -    if (first) {
> +    if (so->first) {
>          *list = entry;
> +        so->first = false;
>      } else {
>          (*list)->next = entry;
>      }

This stuff never fails to confuse me.  But now there's hope it's mostly
because the *old* code is confusing.