Re: [Qemu-devel] [PATCH v14 18/19] qapi: Simplify semantics of visit_next_list()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> 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

Period here, convert the parenthesis to a sentence.

>               (that is, all other list visitors are generated
> in qapi-visit.c, and share the same paradigm based on a qapi
> FooList type).

The above reviews existing uses.  Next, you show that visit_next_list()
semantics are awkward.  Suggest to start with the awkward semantics, and
only then review existing uses.

>                 What's more, the semantics of the list visit are
> somewhat baroque, with the following pseudocode when FooList is
> used:
>
> start()
> for (prev = head; cur = next(prev); prev = &cur) {
>     visit(&cur->value)
> }
>
> 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)
> for (tail = *head; tail; tail = next(tail)) {
>     visit(&tail->value)
> }
>
> 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()).

Minor drawback: we now allocate both in start_list() and in next_list().
Minor, because the allocation is trivial.

> The signature of visit_start_list() is chosen to match
> visit_start_struct(), with the new parameters after 'name'.

Good idea.

> The spapr_drc case is a virtual visit, 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 (several prerequisite patches
> refactored things to make this patch straightforward).  But it
> turned out that the string and opts visitors munge enough other
> state during visit_next_list() to make it easier to just
> document and 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 <eblake@redhat.com>
>
> ---
> v14: no change
> v13: documentation wording tweaks
> v12: rebase to earlier changes, drop R-b, improve documentation,
> split out qmp-input cleanups into prereq patches
> [no v10, v11]
> v9: no change
> v8: consistent parameter order, fix qmp_input_get_next_type() to not
> skip list entries
> v7: new patch
> ---
>  include/qapi/visitor.h               | 47 +++++++++++++++++++-----------------
>  include/qapi/visitor-impl.h          |  7 +++---
>  scripts/qapi-visit.py                | 18 +++++++-------
>  include/qapi/opts-visitor.h          |  2 +-
>  include/qapi/string-input-visitor.h  |  3 ++-
>  include/qapi/string-output-visitor.h |  3 ++-
>  qapi/qapi-visit-core.c               | 12 +++++----
>  hw/ppc/spapr_drc.c                   |  2 +-
>  qapi/opts-visitor.c                  | 33 +++++++++++--------------
>  qapi/qapi-dealloc-visitor.c          | 35 ++++++---------------------
>  qapi/qmp-input-visitor.c             | 32 ++++++++++++------------
>  qapi/qmp-output-visitor.c            | 22 ++++-------------
>  qapi/string-input-visitor.c          | 36 +++++++++++++--------------
>  qapi/string-output-visitor.c         | 41 +++++++++++--------------------
>  docs/qapi-code-gen.txt               | 17 +++++++------
>  15 files changed, 133 insertions(+), 177 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index e1213a3..a22a7ce 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -165,7 +165,7 @@
>   *  if (err) {
>   *      goto outobj;
>   *  }
> - *  visit_start_list(v, "list", &err);
> + *  visit_start_list(v, "list", NULL, 0, &err);
>   *  if (err) {
>   *      goto outobj;
>   *  }
> @@ -268,40 +268,43 @@ void visit_end_struct(Visitor *v);
>   * @name expresses the relationship of this list to its parent
>   * container; see the general description of @name above.
>   *
> + * @list must be non-NULL for a real walk, in which case @size
> + * determines how much memory an input visitor will allocate into
> + * *@list (at least sizeof(GenericList)).  Some visitors also allow
> + * @list to be NULL for a virtual walk, in which case @size is
> + * ignored.
> + *
>   * @errp must be NULL-initialized, and is set if an error is detected
>   * (such as if a member @name is not present, or is present but not a
> - * list).
> + * list).  On error, input visitors set *@obj to NULL.

I guess you copied this from visit_start_struct(), which only makes
sense.  Except the thing is called @list here.  More of the same below.

>   *
>   * After visit_start_list() succeeds, the caller may visit its members
> - * one after the other.  A real visit uses visit_next_list() for
> - * traversing the linked list, while a virtual visit uses other means.
> - * For each list element, call the appropriate visit_type_FOO() with
> - * name set to NULL and obj set to the address of the value member of
> - * the list element.  Finally, visit_end_list() needs to be called to
> - * clean up, even if intermediate visits fail.  See the examples
> - * above.
> + * one after the other.  A real visit (where @obj is non-NULL) uses
> + * visit_next_list() for traversing the linked list, while a virtual
> + * visit (where @obj is NULL) uses other means.  For each list
> + * element, call the appropriate visit_type_FOO() with name set to
> + * NULL and obj set to the address of the value member of the list
> + * element.  Finally, visit_end_list() needs to be called to clean up,
> + * even if intermediate visits fail.  See the examples above.
>   */
> -void visit_start_list(Visitor *v, const char *name, Error **errp);
> +void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +                      size_t size, Error **errp);
>
>  /*
> - * Iterate over a GenericList during a list visit.
> + * Iterate over a GenericList during a non-virtual list visit.

Is "non-virtual" new in this patch?  If not, is this the right patch to
insert it?

>   *
> - * @size represents the size of a linked list node.
> + * @size represents the size of a linked list node (at least
> + * sizeof(GenericList)).

Likewise.

>   *
> - * @list must not be NULL; on the first call, @list contains the
> - * address of the list head, and on subsequent calls *@list must be
> - * the previously returned value.  Must be called in a loop until a
> + * @tail must not be NULL; on the first call, @tail is the value of
> + * *list after visit_start_list(), and on subsequent calls @tail 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 and
> - * obj set to the address of (*@list)->value.
> - *
> - * FIXME: This interface is awkward; it requires all callbacks to
> - * track whether it is the first or a subsequent call.  A better
> - * interface would pass the head of the list through
> - * visit_start_list().
> + * obj set to the address of @tail->value.
>   */
> -GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
> +GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
>
>  /*
>   * Complete a list visit started earlier.
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index d44fcd1..0471465 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -45,11 +45,12 @@ struct Visitor
>      /* Must be set to visit structs.  */
>      void (*end_struct)(Visitor *v);
>
> -    /* Must be set.  */
> -    void (*start_list)(Visitor *v, const char *name, Error **errp);
> +    /* Must be set; document if @list may not be NULL.  */

Too terse, I'm afraid.  Suggest something like "implementations may
require @list to be non-null, but must document it."

> +    void (*start_list)(Visitor *v, const char *name, GenericList **list,
> +                       size_t size, Error **errp);
>
>      /* Must be set.  */
> -    GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
> +    GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
>
>      /* Must be set.  */
>      void (*end_list)(Visitor *v);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index bfe4a09..69f0133 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -117,20 +117,20 @@ 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 *tail;
> +    size_t size = sizeof(**obj);
>
> -    visit_start_list(v, name, &err);
> +    visit_start_list(v, name, (GenericList **)obj, size, &err);
>      if (err) {
>          goto out;
>      }
> -
> -    for (prev = (GenericList **)obj;
> -         !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
> -         prev = &i) {
> -        %(c_name)s *native_i = (%(c_name)s *)i;
> -        visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
> +    for (tail = *obj; tail;
> +         tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) {
> +        visit_type_%(c_elt_type)s(v, NULL, &tail->value, &err);
> +        if (err) {
> +            break;
> +        }
>      }
> -

Keep the blank lines around the loop?

>      visit_end_list(v);
>  out:
>      error_propagate(errp, err);
> diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
> index 3fcd327..7c9a7e5 100644
> --- a/include/qapi/opts-visitor.h
> +++ b/include/qapi/opts-visitor.h
> @@ -32,7 +32,7 @@ typedef struct OptsVisitor OptsVisitor;
>   *
>   * The Opts input visitor does not yet implement support for visiting
>   * QAPI alternates, numbers (other than integers), null, or arbitrary
> - * QTypes.
> + * QTypes.  It also requires non-NULL list to visit_start_list().

non-null list argument to

>   */
>  OptsVisitor *opts_visitor_new(const QemuOpts *opts);
>  void opts_visitor_cleanup(OptsVisitor *nv);
> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
> index 1a34c52..d26a845 100644
> --- a/include/qapi/string-input-visitor.h
> +++ b/include/qapi/string-input-visitor.h
> @@ -19,7 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
>
>  /*
>   * The string input visitor does not yet implement support for
> - * visiting QAPI structs, alternates, null, or arbitrary QTypes.
> + * visiting QAPI structs, alternates, null, or arbitrary QTypes.  It
> + * also requires non-NULL list to visit_start_list().

Likewise.

>   */
>  StringInputVisitor *string_input_visitor_new(const char *str);
>  void string_input_visitor_cleanup(StringInputVisitor *v);
> diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h
> index 2564833..0a9354c 100644
> --- a/include/qapi/string-output-visitor.h
> +++ b/include/qapi/string-output-visitor.h
> @@ -19,7 +19,8 @@ typedef struct StringOutputVisitor StringOutputVisitor;
>
>  /*
>   * The string output visitor does not yet implement support for
> - * visiting QAPI structs, alternates, null, or arbitrary QTypes.
> + * visiting QAPI structs, alternates, null, or arbitrary QTypes.  It
> + * also requires non-NULL list to visit_start_list().

Likewise.

>   */
>  StringOutputVisitor *string_output_visitor_new(bool human);
>  void string_output_visitor_cleanup(StringOutputVisitor *v);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 76ea690..622b315 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -42,15 +42,17 @@ void visit_end_struct(Visitor *v)
>      v->end_struct(v);
>  }
>
> -void visit_start_list(Visitor *v, const char *name, Error **errp)
> +void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +                      size_t size, Error **errp)
>  {
> -    v->start_list(v, name, errp);
> +    assert(!list || size >= sizeof(GenericList));
> +    v->start_list(v, name, list, size, errp);
>  }
>
> -GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
> +GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
>  {
> -    assert(list && size >= sizeof(GenericList));
> -    return v->next_list(v, list, size);
> +    assert(tail && size >= sizeof(GenericList));
> +    return v->next_list(v, tail, size);
>  }
>
>  void visit_end_list(Visitor *v)
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 5395c02..dd3c502 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -309,7 +309,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, 0, &err);
>              if (err) {
>                  error_propagate(errp, err);
>                  return;
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index a08d5a7..6d572cc 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -23,9 +23,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
> @@ -214,35 +213,33 @@ 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, size_t size,
> +                Error **errp)
>  {
>      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 */

without a list

Do we plan to support them?  If not, scratch "yet".

> +    assert(list);
>      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_malloc0(size);
> +    } else {
> +        *list = NULL;
>      }
>  }
>
>
>  static GenericList *
> -opts_next_list(Visitor *v, GenericList **list, size_t size)
> +opts_next_list(Visitor *v, GenericList *tail, size_t size)
>  {
>      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;
> @@ -263,7 +260,6 @@ opts_next_list(Visitor *v, GenericList **list, size_t size)
>              g_hash_table_remove(ov->unprocessed_opts, opt->name);
>              return NULL;
>          }
> -        link = &(*list)->next;
>          break;
>      }
>
> @@ -271,8 +267,8 @@ opts_next_list(Visitor *v, GenericList **list, size_t size)
>          abort();
>      }
>
> -    *link = g_malloc0(size);
> -    return *link;
> +    tail->next = g_malloc0(size);
> +    return tail->next;
>  }
>
>
> @@ -281,8 +277,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;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 9005bad..cd68b55 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -22,7 +22,6 @@
>  typedef struct StackEntry
>  {
>      void *value;
> -    bool is_list_head;
>      QTAILQ_ENTRY(StackEntry) node;
>  } StackEntry;
>
> @@ -43,10 +42,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);
>  }
>
> @@ -93,38 +88,22 @@ static void qapi_dealloc_end_alternate(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, size_t size,
> +                                    Error **errp)
>  {
> -    QapiDeallocVisitor *qov = to_qov(v);
> -    qapi_dealloc_push(qov, NULL);
>  }
>
> -static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
> +static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *tail,
>                                             size_t size)
>  {
> -    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 = tail->next;
> +    g_free(tail);
> +    return next;
>  }
>
>  static void qapi_dealloc_end_list(Visitor *v)
>  {
> -    QapiDeallocVisitor *qov = to_qov(v);
> -    void *obj = qapi_dealloc_pop(qov);
> -    assert(obj == NULL); /* should've been list head tracker with no payload */
>  }
>

One step closer to killing the stack.

>  static void qapi_dealloc_type_str(Visitor *v, const char *name, char **obj,
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 16f2f5a..5360744 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -29,8 +29,6 @@ typedef struct StackObject
>
>      /* If obj is list: tail of list still needing visits */
>      const QListEntry *entry;
> -    /* If obj is list: true if head is not visited yet */
> -    bool first;
>
>      GHashTable *h; /* If obj is dict: remaining keys needing visits */
>  } StackObject;
> @@ -87,7 +85,6 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>          assert(!name);
>
>          if (tos->entry) {
> -            assert(!tos->first);
>              qobj = qlist_entry_obj(tos->entry);
>              if (consume) {
>                  tos->entry = qlist_next(tos->entry);
> @@ -117,7 +114,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
>
>      tos->obj = obj;
>      tos->entry = entry;
> -    tos->first = true;
>      tos->h = NULL;
>
>      if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> @@ -194,13 +190,17 @@ static void qmp_input_start_struct(Visitor *v, const char *name, 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, size_t size, 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) {
> +        if (list) {
> +            *list = NULL;
> +        }
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "list");
>          return;
> @@ -208,28 +208,26 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>
>      entry = qlist_first(qobject_to_qlist(qobj));
>      qmp_input_push(qiv, qobj, entry, errp);
> +    if (list) {
> +        if (entry) {
> +            *list = g_malloc0(size);
> +        } else {
> +            *list = NULL;
> +        }
> +    }
>  }
>
> -static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
> +static GenericList *qmp_input_next_list(Visitor *v, GenericList *tail,
>                                          size_t size)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> -    GenericList *entry;
>      StackObject *so = &qiv->stack[qiv->nb_stack - 1];
>
>      if (!so->entry) {
>          return NULL;
>      }
> -
> -    entry = g_malloc0(size);
> -    if (so->first) {
> -        *list = entry;
> -        so->first = false;
> -    } else {
> -        (*list)->next = entry;
> -    }
> -
> -    return entry;
> +    tail->next = g_malloc0(size);
> +    return tail->next;
>  }
>
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index ecb2005..40657be 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -22,7 +22,6 @@
>  typedef struct QStackEntry
>  {
>      QObject *value;
> -    bool is_list_head;
>      QTAILQ_ENTRY(QStackEntry) node;
>  } QStackEntry;
>
> @@ -52,9 +51,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);
>  }
>
> @@ -120,7 +116,9 @@ static void qmp_output_end_struct(Visitor *v)
>      assert(qobject_type(value) == 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, size_t size,
> +                                  Error **errp)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
>      QList *list = qlist_new();
> @@ -129,20 +127,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 *tail,
>                                           size_t size)
>  {
> -    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 tail->next;
>  }
>
>  static void qmp_output_end_list(Visitor *v)
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 797973a..77dd1a7 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -25,8 +25,6 @@ struct StringInputVisitor
>  {
>      Visitor visitor;
>
> -    bool head;
> -
>      GList *ranges;
>      GList *cur_range;
>      int64_t cur;
> @@ -125,11 +123,21 @@ error:
>  }
>
>  static void
> -start_list(Visitor *v, const char *name, Error **errp)
> +start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> +           Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
> +    Error *err = NULL;
>
> -    parse_str(siv, errp);
> +    /* We don't support visits without a GenericList, yet */

without a list

Do we plan to support them?  If not, scratch "yet".

> +    assert(list);
> +
> +    parse_str(siv, &err);
> +    if (err) {
> +        *list = NULL;
> +        error_propagate(errp, err);
> +        return;
> +    }
>
>      siv->cur_range = g_list_first(siv->ranges);
>      if (siv->cur_range) {
> @@ -137,13 +145,15 @@ start_list(Visitor *v, const char *name, Error **errp)
>          if (r) {
>              siv->cur = r->begin;
>          }
> +        *list = g_malloc0(size);
> +    } else {
> +        *list = NULL;
>      }
>  }
>
> -static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
> +static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>  {
>      StringInputVisitor *siv = to_siv(v);
> -    GenericList **link;
>      Range *r;
>
>      if (!siv->ranges || !siv->cur_range) {
> @@ -167,21 +177,12 @@ static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
>          siv->cur = r->begin;
>      }
>
> -    if (siv->head) {
> -        link = list;
> -        siv->head = false;
> -    } else {
> -        link = &(*list)->next;
> -    }
> -
> -    *link = g_malloc0(size);
> -    return *link;
> +    tail->next = g_malloc0(size);
> +    return tail->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,
> @@ -362,6 +363,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 0d44d7e..e27e2df 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -20,7 +20,7 @@
>
>  enum ListMode {
>      LM_NONE,             /* not traversing a list of repeated options */
> -    LM_STARTED,          /* start_list() succeeded */
> +    LM_STARTED,          /* start_list() succeeded with multiple elements */

In opts-visitor.c, the comment says "opts_next_list() ready to be
called".  Any reason for the difference?

>
>      LM_IN_PROGRESS,      /* next_list() has been called.
>                            *
> @@ -48,7 +48,7 @@ enum ListMode {
>
>      LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */
>
> -    LM_END
> +    LM_END,              /* next_list() called, about to see last element. */
>  };
>
>  typedef enum ListMode ListMode;
> @@ -58,7 +58,6 @@ struct StringOutputVisitor
>      Visitor visitor;
>      bool human;
>      GString *string;
> -    bool head;
>      ListMode list_mode;
>      union {
>          int64_t s;
> @@ -266,39 +265,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, size_t size,
> +           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 */

without a list

Do we plan to support them?  If not, scratch "yet".

> +    assert(list);
> +    /* List handling is only needed if there are at least two elements */
> +    if (*list && (*list)->next) {
> +        sov->list_mode = LM_STARTED;
> +    }
>  }
>
> -static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
> +static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>  {
>      StringOutputVisitor *sov = to_sov(v);
> -    GenericList *ret = NULL;
> -    if (*list) {
> -        if (sov->head) {
> -            ret = *list;
> -        } else {
> -            ret = (*list)->next;
> -        }
> +    GenericList *ret = tail->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;
>  }
>
> @@ -311,8 +300,6 @@ static void 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)
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 3b2422f..99080db 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -912,18 +912,19 @@ Example:
>      void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp)
>      {
>          Error *err = NULL;
> -        GenericList *i, **prev;
> +        UserDefOneList *tail;
> +        size_t size = sizeof(**obj);
>
> -        visit_start_list(v, name, &err);
> +        visit_start_list(v, name, (GenericList **)obj, size, &err);
>          if (err) {
>              goto out;
>          }
> -
> -        for (prev = (GenericList **)obj;
> -             !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
> -             prev = &i) {
> -            UserDefOneList *native_i = (UserDefOneList *)i;
> -            visit_type_UserDefOne(v, NULL, &native_i->value, &err);
> +        for (tail = *obj; tail;
> +             tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail, size)) {
> +            visit_type_UserDefOne(v, NULL, &tail->value, &err);
> +            if (err) {
> +                break;
> +            }
>          }
>
>          visit_end_list(v);