Re: [Qemu-devel] [PATCH v9 34/37] qapi: Simplify semantics of visit_next_list()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> We have two uses of list visits in the entire code base: one in
> spapr_drc (which completely avoids visit_next_list(), feeding in
> integers from a different source than uint8List), and one in
> qapi-visit.py (that is, all other list visitors are generated
> in qapi-visit.c, and share the same paradigm based on a qapi
> FooList type).  What's more, the semantics of the list visit are
> somewhat baroque, with the following pseudocode when FooList is
> used:
>
> start()
> prev = head
> while (cur = next(prev)) {
>     visit(cur)

Actually, we pass &cur->value to the element visit.

>     prev = &cur
> }
>
> Note that these semantics (advance before visit) requires that
> the first call to next() return the list head, while all other
> calls return the next element of the list; that is, every visitor
> implementation is required to track extra state to decide whether
> to return the input as-is, or to advance.  It also requires an
> argument of 'GenericList **' to next(), solely because the first
> iteration might need to modify the caller's GenericList head, so
> that all other calls have to do a layer of dereferencing.
>
> We can greatly simplify things by hoisting the special case
> into the start() routine, and flipping the order in the loop
> to visit before advance:
>
> start(head)
> element = *head
> while (element) {
>     visit(element)
>     element = next(element)
> }

@element isn't a list element, it's a list node.  Suggest

  start(head)
  tail = *head
  while (tail) {
      visit(&tail->value)
      tail = next(tail)
  }

Of course, this pseudo-code just screams to be a for-loop instead:

  for (tail = *head; tail; tail = next(tail)) ...

May or may not be an improvement of the real code.  The pseudo-code
should follow the real code.

> With the simpler semantics, visitors have less state to track,
> the argument to next() is reduced to 'GenericList *', and it
> also becomes obvious whether an input visitor is allocating a
> FooList during visit_start_list() (rather than the old way of
> not knowing if an allocation happened until the first
> visit_next_list()).
>
> The signature of visit_start_list() is chosen to match
> visit_start_struct(), with the new parameter after 'name'.
>
> The spapr_drc case requires that visit_start_list() has to pay
> attention to whether visit_next_list() will even be used to
> visit a FooList qapi struct; this is done by passing NULL for
> list, similarly to how NULL is passed to visit_start_struct()
> when a qapi type is not used in those visits.  It was easy to
> provide these semantics for qmp-output and dealloc visitors,
> and a bit harder for qmp-input (it required hoisting the
> advance of the current qlist entry out of qmp_input_next_list()
> into qmp_input_get_object()).  But it turned out that the
> string and opts visitors munge enough state during
> visit_next_list() to make those conversions simpler if they
> require a GenericList visit for now; an assertion will remind
> us to adjust things if we need the semantics in the future.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v9: no change
> v8: consistent parameter order, fix qmp_input_get_next_type() to not
> skip list entries
> v7: new patch
> ---
>  hw/ppc/spapr_drc.c           |  2 +-
>  include/qapi/visitor-impl.h  |  5 ++--
>  include/qapi/visitor.h       | 45 +++++++++++++++++--------------
>  qapi/opts-visitor.c          | 32 +++++++++-------------
>  qapi/qapi-dealloc-visitor.c  | 29 +++++---------------
>  qapi/qapi-visit-core.c       |  7 ++---
>  qapi/qmp-input-visitor.c     | 64 
> +++++++++++++++++++++-----------------------
>  qapi/qmp-output-visitor.c    | 21 +++------------
>  qapi/string-input-visitor.c  | 34 +++++++++++------------
>  qapi/string-output-visitor.c | 36 ++++++++-----------------
>  scripts/qapi-visit.py        | 21 ++++++++-------
>  11 files changed, 126 insertions(+), 170 deletions(-)

Diffstat suggests it's indeed a simplification.

>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 3b27caa..41f2da0 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -299,7 +299,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
> char *name,
>              int i;
>              prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
>              name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
> -            visit_start_list(v, name, &err);
> +            visit_start_list(v, name, NULL, &err);
>              if (err) {
>                  error_propagate(errp, err);
>                  return;
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 248b1e5..acbe7d6 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -40,9 +40,10 @@ struct Visitor
>      void (*end_implicit_struct)(Visitor *v);
>
>      /* Must be set */
> -    void (*start_list)(Visitor *v, const char *name, Error **errp);
> +    void (*start_list)(Visitor *v, const char *name, GenericList **list,
> +                       Error **errp);
>      /* Must be set */
> -    GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
> +    GenericList *(*next_list)(Visitor *v, GenericList *element, Error 
> **errp);

You rename the parameter here, but not in the implementations.

The parameter isn't a list element, it's a list node.  If we want to
rename it, what about tail?

>      /* Must be set */
>      void (*end_list)(Visitor *v);
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index e5dcde4..4638863 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -1,6 +1,7 @@
>  /*
>   * Core Definitions for QAPI Visitor Classes
>   *
> + * Copyright (C) 2015 Red Hat, Inc.

It's 2016 now.  I'd make it 2012-2016.

>   * Copyright IBM, Corp. 2011
>   *
>   * Authors:
> @@ -111,32 +112,36 @@ void visit_end_implicit_struct(Visitor *v);
>  /**
>   * Prepare to visit a list tied to an object key @name.
>   * @name will be NULL if this is visited as part of another list.
> - * After calling this, the elements must be collected until
> - * visit_next_list() returns NULL, then visit_end_list() must be
> - * used to complete the visit.
> - */
> -void visit_start_list(Visitor *v, const char *name, Error **errp);
> -/**
> - * Iterate over a GenericList during a list visit.
> - * @list must not be NULL; on the first call, @list contains the
> - * address of the list head, and on subsequent calls address@hidden must be
> - * the previously returned value.  Must be called in a loop until a
> - * NULL return or error occurs; for each non-NULL return, the caller
> - * must then call the appropriate visit_type_*() for the element type
> - * of the list, with that function's name parameter set to NULL.
> + * Input visitors malloc a qapi List struct into address@hidden,

QAPI

What's a QAPI list struct?  I guess you mean GenericList.

>                                                           or set it to
> + * NULL if there are no elements in the list;

So start_list() now needs to know whether the list is empty.

visit_start_struct() has an "if @obj is not NULL" clause here.  Do we
need the equivalent "if @list is not NULL"?  Ah, you do that further
down!  Can we structure the two comments the same way?

>                                                and output visitors
> + * expect address@hidden to point to the start of the list, if any.

Perhaps "to the first list node, if any", and give GenericList a better
comment in PATCH 21:

    /*
     * Generic list node
     * Any generated QAPI FOOList struct pointer can be safely cast to
     * GenericList * and dereferenced.
     */

Of course, this actually assumes uniform pointer representation, which
is not guaranteed by the standard.

Should we mention output visitors don't change address@hidden  Same for
visit_start_struct(), by the way.

What about the dealloc visitor?

>                                                               On
> + * return, if address@hidden is non-NULL, the caller should enter a loop
> + * visiting the current element, then using visit_next_list() to
> + * advance to the next element, until that returns NULL; then
> + * visit_end_list() must be used to complete the visit.

For visit_start_struct() this part reads:

                                                             The
 * caller then makes a series of visit calls for each key expected in
 * the object, where those visits set their respective obj parameter
 * to the address of a member of the qapi struct, and follows
 * everything by a call to visit_end_struct() to clean up resources.

Following that pattern here, I get:

   The caller then visits the list elements in turn, where those visits
   get passed the address of the list element within the QAPI list node.
   The caller normally uses visit_next_list() to step through the list.
   When done, it must call visit_end_list() to clean up.

>   *
> - * Note that for some visitors (qapi-dealloc and qmp-output), when a
> - * qapi GenericList linked list is not being used (comparable to when
> - * a NULL obj is used for visit_start_struct()), it is acceptable to
> - * bypass the use of visit_next_list() and just directly call the
> - * appropriate visit_type_*() for each element in between the
> - * visit_start_list() and visit_end_list() calls.
> + * If supported by a visitor, @list can be NULL to indicate that there

by the visitor

> + * is no qapi List struct, and that the upcoming visit calls are
> + * parsing input to or creating output from some other representation;
> + * in this case, visit_next_list() will not be needed, but
> + * visit_end_list() is still mandatory.

You first explain normal usage, then add this paragraph to explain
special usage.  I like that better than the visit_start_struct()
comment, where the special usage comes earlier.

>   *
>   * FIXME: For input visitors, address@hidden can be assigned here even if
>   * later visits will fail; this can lead to memory leaks if clients
>   * aren't careful.
>   */
> -GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
> +void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +                      Error **errp);
> +/**
> + * Iterate over a GenericList during a list visit.
> + * Before calling this function, the caller should use the appropriate
> + * visit_type_FOO() for the current list element at @element->value, and
> + * check for errors. @element must not be NULL; on the first iteration,
> + * it should be the value in *list after visit_start_list(); on other
> + * calls it should be the previous return value.  This function
> + * returns NULL once there are no further list elements.
> + */

I feel if we get visit_start_list()'s comment right, then this one can
concentrate on what this function does, and leave intended usage of the
start/next/end team to visit_start_list()'s comment.

> +GenericList *visit_next_list(Visitor *v, GenericList *element, Error **errp);
>  /**
>   * Complete the list started earlier.
>   * Must be called after any successful use of visit_start_list(),
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index b469573..c5a7396 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -21,9 +21,8 @@
>  enum ListMode
>  {
>      LM_NONE,             /* not traversing a list of repeated options */
> -    LM_STARTED,          /* opts_start_list() succeeded */
>
> -    LM_IN_PROGRESS,      /* opts_next_list() has been called.
> +    LM_IN_PROGRESS,      /* opts_next_list() ready to be called.
>                            *
>                            * Generating the next list link will consume the 
> most
>                            * recently parsed QemuOpt instance of the repeated
> @@ -212,35 +211,32 @@ lookup_distinct(const OptsVisitor *ov, const char 
> *name, Error **errp)
>
>
>  static void
> -opts_start_list(Visitor *v, const char *name, Error **errp)
> +opts_start_list(Visitor *v, const char *name, GenericList **list, Error 
> **errp)

I'd break this line before Error.

>  {
>      OptsVisitor *ov = to_ov(v);
>
>      /* we can't traverse a list in a list */
>      assert(ov->list_mode == LM_NONE);
> +    /* we don't support visits without GenericList, yet */
> +    assert(list);

Aha, this is why you wrote "If supported by a visitor".  The visitors
better document what they support then.

>      ov->repeated_opts = lookup_distinct(ov, name, errp);
> -    if (ov->repeated_opts != NULL) {
> -        ov->list_mode = LM_STARTED;
> +    if (ov->repeated_opts) {
> +        ov->list_mode = LM_IN_PROGRESS;
> +        *list = g_new0(GenericList, 1);
> +    } else {
> +        *list = NULL;
>      }
>  }
>
>
>  static GenericList *
> -opts_next_list(Visitor *v, GenericList **list, Error **errp)
> +opts_next_list(Visitor *v, GenericList *list, Error **errp)
>  {
>      OptsVisitor *ov = to_ov(v);
> -    GenericList **link;
>
>      switch (ov->list_mode) {
> -    case LM_STARTED:
> -        ov->list_mode = LM_IN_PROGRESS;
> -        link = list;
> -        break;
> -
>      case LM_SIGNED_INTERVAL:
>      case LM_UNSIGNED_INTERVAL:
> -        link = &(*list)->next;
> -
>          if (ov->list_mode == LM_SIGNED_INTERVAL) {
>              if (ov->range_next.s < ov->range_limit.s) {
>                  ++ov->range_next.s;
> @@ -261,7 +257,6 @@ opts_next_list(Visitor *v, GenericList **list, Error 
> **errp)
>              g_hash_table_remove(ov->unprocessed_opts, opt->name);
>              return NULL;
>          }
> -        link = &(*list)->next;
>          break;
>      }
>
> @@ -269,8 +264,8 @@ opts_next_list(Visitor *v, GenericList **list, Error 
> **errp)
>          abort();
>      }
>
> -    *link = g_malloc0(sizeof **link);
> -    return *link;
> +    list->next = g_new0(GenericList, 1);
> +    return list->next;
>  }
>
>
> @@ -279,8 +274,7 @@ opts_end_list(Visitor *v)
>  {
>      OptsVisitor *ov = to_ov(v);
>
> -    assert(ov->list_mode == LM_STARTED ||
> -           ov->list_mode == LM_IN_PROGRESS ||
> +    assert(ov->list_mode == LM_IN_PROGRESS ||
>             ov->list_mode == LM_SIGNED_INTERVAL ||
>             ov->list_mode == LM_UNSIGNED_INTERVAL);
>      ov->repeated_opts = NULL;

Only slight simplification for this visitor: we lose LM_STARTED.

> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index a89e6d1..839049e 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -20,7 +20,6 @@
>  typedef struct StackEntry
>  {
>      void *value;
> -    bool is_list_head;
>      QTAILQ_ENTRY(StackEntry) node;
>  } StackEntry;
>
> @@ -41,10 +40,6 @@ static void qapi_dealloc_push(QapiDeallocVisitor *qov, 
> void *value)
>
>      e->value = value;
>
> -    /* see if we're just pushing a list head tracker */
> -    if (value == NULL) {
> -        e->is_list_head = true;
> -    }
>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>  }
>
> @@ -92,31 +87,19 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v)
>      }
>  }
>
> -static void qapi_dealloc_start_list(Visitor *v, const char *name, Error 
> **errp)
> +static void qapi_dealloc_start_list(Visitor *v, const char *name,
> +                                    GenericList **list, Error **errp)
>  {
>      QapiDeallocVisitor *qov = to_qov(v);
>      qapi_dealloc_push(qov, NULL);

Do we still need to push/pop for a list?

If yes, can we push list instead of NULL?  Pointers always become more
complicated when they can be null...

>  }
>
> -static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
> +static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list,
>                                             Error **errp)
>  {
> -    GenericList *list = *listp;
> -    QapiDeallocVisitor *qov = to_qov(v);
> -    StackEntry *e = QTAILQ_FIRST(&qov->stack);
> -
> -    if (e && e->is_list_head) {
> -        e->is_list_head = false;
> -        return list;
> -    }
> -
> -    if (list) {
> -        list = list->next;
> -        g_free(*listp);
> -        return list;
> -    }
> -
> -    return NULL;
> +    GenericList *next = list->next;
> +    g_free(list);
> +    return next;
>  }

Okay, this is actually a more worthwhile simplification.

>
>  static void qapi_dealloc_end_list(Visitor *v)
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 9506a02..f391a70 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -67,12 +67,13 @@ void visit_end_implicit_struct(Visitor *v)
>      }
>  }
>
> -void visit_start_list(Visitor *v, const char *name, Error **errp)
> +void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +                      Error **errp)
>  {
> -    v->start_list(v, name, errp);
> +    v->start_list(v, name, list, errp);
>  }
>
> -GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
> +GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp)
>  {
>      assert(list);
>      return v->next_list(v, list, errp);
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index f256d9e..82f9333 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -44,16 +44,20 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>                                       const char *name,
>                                       bool consume)
>  {
> -    QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
> +    StackObject *so = &qiv->stack[qiv->nb_stack - 1];
> +    QObject *qobj = so->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);
> +            if (so->h && consume) {
> +                g_hash_table_remove(so->h, name);
> +            }
> +            qobj = qdict_get(qobject_to_qdict(qobj), name);
> +        } else if (so->entry) {
> +            qobj = qlist_entry_obj(so->entry);
> +            if (consume) {
> +                so->entry = qlist_next(so->entry);
>              }
> -            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);
>          }
>      }
>

Much easier to read if split into two steps:

1. Capture the pointer to the top of the stack in a variable

@@ -44,16 +44,17 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
                                      const char *name,
                                      bool consume)
 {
-    QObject *qobj = qiv->stack[qiv->nb_stack - 1].obj;
+    StackObject *so = &qiv->stack[qiv->nb_stack - 1];
+    QObject *qobj = so->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);
+            if (so->h && consume) {
+                g_hash_table_remove(so->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);
+        } else if (so->entry) {
+            return qlist_entry_obj(so->entry);
         }
     }
 
2. Make the actual change

@@ -52,9 +52,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
             if (so->h && consume) {
                 g_hash_table_remove(so->h, name);
             }
-            return qdict_get(qobject_to_qdict(qobj), name);
+            qobj = qdict_get(qobject_to_qdict(qobj), name);
         } else if (so->entry) {
-            return qlist_entry_obj(so->entry);
+            qobj = qlist_entry_obj(so->entry);
+            if (consume) {
+                so->entry = qlist_next(so->entry);
+            }
         }
     }
 
     return qobj;
 
I'd call the new variable tos (top of stack) instead of so.  Needs a
matching rename in qmp_input_next_list().

Note that @consume is true unless we're called from
qmp_input_get_next_type().  What is it supposed to do?  Can it be true
in the place where you add a use?

Since we're cleaning up the function anyway in step 1, I'd be tempted to
reduce its nesting:

    if (!qobj) {
        return NULL;
    } else 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);
    } else if (tos->entry) {
        return qlist_entry_obj(tos->entry);
    } else {
        return qobj;
    }

Immediately begs the question what the four cases mean.  Unobvious
enough to justify comments, I think.

What's the stack's contents?  You cleaned that up and documented it in
qmp-output-visitor.c.  Same treatment here?

> @@ -66,7 +70,8 @@ static void qdict_add_key(const char *key, QObject *obj, 
> void *opaque)
>      g_hash_table_insert(h, (gpointer) key, NULL);
>  }
>
> -static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
> +static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
> +                           const QListEntry *entry, Error **errp)
>  {
>      GHashTable *h;
>
       if (qiv->nb_stack >= QIV_STACK_SIZE) {
           error_setg(errp, "An internal buffer overran");
           return;
       }

Aside: this is stupid, realloc() exists.  It's less stupid than the
qmp-output-visitor.c's tail queue, though.

> @@ -76,7 +81,7 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject 
> *obj, Error **errp)
>      }
>
>      qiv->stack[qiv->nb_stack].obj = obj;
> -    qiv->stack[qiv->nb_stack].entry = NULL;
> +    qiv->stack[qiv->nb_stack].entry = entry;
>      qiv->stack[qiv->nb_stack].h = NULL;
>
>      if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> @@ -138,7 +143,7 @@ static void qmp_input_start_struct(Visitor *v, const char 
> *name, void **obj,
>          return;
>      }
>
> -    qmp_input_push(qiv, qobj, &err);
> +    qmp_input_push(qiv, qobj, NULL, &err);
>      if (err) {
>          error_propagate(errp, err);
>          return;

The stack entry for a struct being visited has obj = its source QDict,
entry = NULL.

> @@ -158,10 +163,12 @@ static void qmp_input_start_implicit_struct(Visitor *v, 
> void **obj,
>      }
>  }
>
> -static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
> +static void qmp_input_start_list(Visitor *v, const char *name,
> +                                 GenericList **list, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      QObject *qobj = qmp_input_get_object(qiv, name, true);
> +    const QListEntry *entry;
>
>      if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> @@ -169,37 +176,28 @@ static void qmp_input_start_list(Visitor *v, const char 
> *name, Error **errp)
>          return;
>      }
>
> -    qmp_input_push(qiv, qobj, errp);
> +    entry = qlist_first(qobject_to_qlist(qobj));
> +    qmp_input_push(qiv, qobj, entry, errp);

The stack entry for a list being visited has obj = its source QList,
entry = NULL.

> +    if (list) {
> +        if (entry) {
> +            *list = g_new0(GenericList, 1);
> +        } else {
> +            *list = NULL;
> +        }
> +    }
>  }
>
> -static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
> +static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
>                                          Error **errp)
>  {
>      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(sizeof(*entry));
> -    if (first) {
> -        *list = entry;
> -    } else {
> -        (*list)->next = entry;
> -    }
> -
> -    return entry;
> +    list->next = g_new0(GenericList, 1);
> +    return list->next;
>  }
>
>
> @@ -375,7 +373,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.optional = qmp_input_optional;
>      v->visitor.get_next_type = qmp_input_get_next_type;
>
> -    qmp_input_push(v, obj, NULL);
> +    qmp_input_push(v, obj, NULL, NULL);

The stack entry for the root value being visited has obj = its source
QObject, entry = NULL.

>      qobject_incref(obj);
>
>      return v;
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 5376948..913f378 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -21,7 +21,6 @@
>  typedef struct QStackEntry
>  {
>      QObject *value;
> -    bool is_list_head;
>      QTAILQ_ENTRY(QStackEntry) node;
>  } QStackEntry;
>
> @@ -51,9 +50,6 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, 
> QObject *value)
>      assert(qov->root);
>      assert(value);
>      e->value = value;
> -    if (qobject_type(e->value) == QTYPE_QLIST) {
> -        e->is_list_head = true;
> -    }
>      QTAILQ_INSERT_HEAD(&qov->stack, e, node);
>  }
>
> @@ -122,7 +118,8 @@ static void qmp_output_end_struct(Visitor *v)
>      qmp_output_pop(qov, QTYPE_QDICT);
>  }
>
> -static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
> +static void qmp_output_start_list(Visitor *v, const char *name,
> +                                  GenericList **listp, Error **errp)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
>      QList *list = qlist_new();
> @@ -131,20 +128,10 @@ static void qmp_output_start_list(Visitor *v, const 
> char *name, Error **errp)
>      qmp_output_push(qov, list);
>  }
>
> -static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
> +static GenericList *qmp_output_next_list(Visitor *v, GenericList *list,
>                                           Error **errp)
>  {
> -    GenericList *list = *listp;
> -    QmpOutputVisitor *qov = to_qov(v);
> -    QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> -
> -    assert(e);
> -    if (e->is_list_head) {
> -        e->is_list_head = false;
> -        return list;
> -    }
> -
> -    return list ? list->next : NULL;
> +    return list->next;
>  }
>
>  static void qmp_output_end_list(Visitor *v)

A simple one, for a change.

> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 610c233..582a62a 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -23,8 +23,6 @@ struct StringInputVisitor
>  {
>      Visitor visitor;
>
> -    bool head;
> -
>      GList *ranges;
>      GList *cur_range;
>      int64_t cur;
> @@ -123,11 +121,19 @@ error:
>  }
>
>  static void
> -start_list(Visitor *v, const char *name, Error **errp)
> +start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
> +    Error *err = NULL;
>
> -    parse_str(siv, errp);
> +    /* We don't support visits without a GenericList, yet */
> +    assert(list);
> +
> +    parse_str(siv, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }

Should you set address@hidden = NULL on error?

Does this handle parse_str() error correctly before your patch?

>
>      siv->cur_range = g_list_first(siv->ranges);
>      if (siv->cur_range) {
> @@ -135,14 +141,16 @@ start_list(Visitor *v, const char *name, Error **errp)
>          if (r) {
>              siv->cur = r->begin;
>          }
> +        *list = g_new0(GenericList, 1);
> +    } else {
> +        *list = NULL;
>      }
>  }

If it does, then this must be the reason you have to bail out on error.

>
>  static GenericList *
> -next_list(Visitor *v, GenericList **list, Error **errp)
> +next_list(Visitor *v, GenericList *list, Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
> -    GenericList **link;
>      Range *r;
>
>      if (!siv->ranges || !siv->cur_range) {
> @@ -166,22 +174,13 @@ next_list(Visitor *v, GenericList **list, Error **errp)
>          siv->cur = r->begin;
>      }
>
> -    if (siv->head) {
> -        link = list;
> -        siv->head = false;
> -    } else {
> -        link = &(*list)->next;
> -    }
> -
> -    *link = g_malloc0(sizeof **link);
> -    return *link;
> +    list->next = g_new0(GenericList, 1);
> +    return list->next;
>  }
>
>  static void
>  end_list(Visitor *v)
>  {
> -    StringInputVisitor *siv = to_siv(v);
> -    siv->head = true;
>  }
>
>  static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
> @@ -361,6 +360,5 @@ StringInputVisitor *string_input_visitor_new(const char 
> *str)
>      v->visitor.optional = parse_optional;
>
>      v->string = str;
> -    v->head = true;
>      return v;
>  }
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index fd917a4..7209d80 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -57,7 +57,6 @@ struct StringOutputVisitor
>      Visitor visitor;
>      bool human;
>      GString *string;
> -    bool head;
>      ListMode list_mode;
>      union {
>          int64_t s;
> @@ -265,40 +264,29 @@ static void print_type_number(Visitor *v, const char 
> *name, double *obj,
>  }
>
>  static void
> -start_list(Visitor *v, const char *name, Error **errp)
> +start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
>
>      /* we can't traverse a list in a list */
>      assert(sov->list_mode == LM_NONE);
> -    sov->list_mode = LM_STARTED;
> -    sov->head = true;
> +    /* We don't support visits without a GenericList, yet */
> +    assert(list);
> +    /* List handling is only needed if there are at least two elements */
> +    if (*list && (*list)->next) {
> +        sov->list_mode = LM_STARTED;
> +    }

Contradicts the comment next to LM_STARTED: /* start_list() succeeded */

Why do you need to stay in state LM_NONE for shorter lists now?


>  }
>
>  static GenericList *
> -next_list(Visitor *v, GenericList **list, Error **errp)
> +next_list(Visitor *v, GenericList *list, Error **errp)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> -    GenericList *ret = NULL;
> -    if (*list) {
> -        if (sov->head) {
> -            ret = *list;
> -        } else {
> -            ret = (*list)->next;
> -        }
> +    GenericList *ret = list->next;
>
> -        if (sov->head) {
> -            if (ret && ret->next == NULL) {
> -                sov->list_mode = LM_NONE;
> -            }
> -            sov->head = false;
> -        } else {
> -            if (ret && ret->next == NULL) {
> -                sov->list_mode = LM_END;
> -            }
> -        }
> +    if (ret && !ret->next) {
> +        sov->list_mode = LM_END;
>      }
> -
>      return ret;
>  }

What does state LM_END mean?  It has no comment...

>
> @@ -312,8 +300,6 @@ end_list(Visitor *v)
>             sov->list_mode == LM_NONE ||
>             sov->list_mode == LM_IN_PROGRESS);
>      sov->list_mode = LM_NONE;
> -    sov->head = true;
> -
>  }
>
>  char *string_output_get_string(StringOutputVisitor *sov)

Unless I'm blind, no next_list() implementation sets an error.  Drop
parameter errp and simplify?

> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8039b97..6016734 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -125,20 +125,23 @@ def gen_visit_list(name, element_type):
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
> Error **errp)
>  {
>      Error *err = NULL;
> -    GenericList *i, **prev;
> +    %(c_name)s *elt;

This isn't an element, it's a list node.  I'd call it tail.

Sure changing its type from GenericList to the specific one saves casts?

>
> -    visit_start_list(v, name, &err);
> +    visit_start_list(v, name, (GenericList **)obj, &err);
>      if (err) {
>          goto out;
>      }
> -
> -    for (prev = (GenericList **)obj;
> -         !err && (i = visit_next_list(v, prev, &err)) != NULL;
> -         prev = &i) {
> -        %(c_name)s *native_i = (%(c_name)s *)i;
> -        visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
> +    elt = *obj;
> +    while (elt) {
> +        visit_type_%(c_elt_type)s(v, NULL, &elt->value, &err);
> +        if (err) {
> +            break;
> +        }
> +        elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, &err);
> +        if (err) {
> +            break;
> +        }
>      }

With a simplified next_list(), this could be a nice for-loop:

       for (tail = *obj; tail; tail = visit_next_list(v, tail, &err)) ...

> -
>      visit_end_list(v);
>  out:
>      error_propagate(errp, err);