From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOmli-0000nP-Bn for qemu-devel@nongnu.org; Thu, 28 Jan 2016 08:37:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOmle-0003rk-QP for qemu-devel@nongnu.org; Thu, 28 Jan 2016 08:37:50 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-35-git-send-email-eblake@redhat.com> Date: Thu, 28 Jan 2016 14:37:41 +0100 In-Reply-To: <1453219845-30939-35-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:42 -0700") Message-ID: <87si1hakpm.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 34/37] qapi: Simplify semantics of visit_next_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Michael Roth , Alexander Graf , qemu-devel@nongnu.org, "open list:sPAPR" , marcandre.lureau@redhat.com, David Gibson Eric Blake 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 =3D head > while (cur =3D next(prev)) { > visit(cur) Actually, we pass &cur->value to the element visit. > prev =3D &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 =3D *head > while (element) { > visit(element) > element =3D next(element) > } @element isn't a list element, it's a list node. Suggest start(head) tail =3D *head while (tail) { visit(&tail->value) tail =3D next(tail) } Of course, this pseudo-code just screams to be a for-loop instead: for (tail =3D *head; tail; tail =3D 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 > Reviewed-by: Marc-Andr=C3=A9 Lureau > > --- > 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, con= st char *name, > int i; > prop =3D fdt_get_property_by_offset(fdt, fdt_offset, &prop_l= en); > name =3D 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 **er= rp); > + 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 *@list 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 *@list, 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 *@list 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 *@list? Same for visit_start_struct(), by the way. What about the dealloc visitor? > On > + * return, if *@list 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, *@list 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 **err= p); > +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 **e= rrp); > /** > * 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 t= he most > * recently parsed QemuOpt instance of the repe= ated > @@ -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 =3D to_ov(v); > > /* we can't traverse a list in a list */ > assert(ov->list_mode =3D=3D 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 =3D lookup_distinct(ov, name, errp); > - if (ov->repeated_opts !=3D NULL) { > - ov->list_mode =3D LM_STARTED; > + if (ov->repeated_opts) { > + ov->list_mode =3D LM_IN_PROGRESS; > + *list =3D g_new0(GenericList, 1); > + } else { > + *list =3D NULL; > } > } > > > static GenericList * > -opts_next_list(Visitor *v, GenericList **list, Error **errp) > +opts_next_list(Visitor *v, GenericList *list, Error **errp) > { > OptsVisitor *ov =3D to_ov(v); > - GenericList **link; > > switch (ov->list_mode) { > - case LM_STARTED: > - ov->list_mode =3D LM_IN_PROGRESS; > - link =3D list; > - break; > - > case LM_SIGNED_INTERVAL: > case LM_UNSIGNED_INTERVAL: > - link =3D &(*list)->next; > - > if (ov->list_mode =3D=3D 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 =3D &(*list)->next; > break; > } > > @@ -269,8 +264,8 @@ opts_next_list(Visitor *v, GenericList **list, Error = **errp) > abort(); > } > > - *link =3D g_malloc0(sizeof **link); > - return *link; > + list->next =3D g_new0(GenericList, 1); > + return list->next; > } > > > @@ -279,8 +274,7 @@ opts_end_list(Visitor *v) > { > OptsVisitor *ov =3D to_ov(v); > > - assert(ov->list_mode =3D=3D LM_STARTED || > - ov->list_mode =3D=3D LM_IN_PROGRESS || > + assert(ov->list_mode =3D=3D LM_IN_PROGRESS || > ov->list_mode =3D=3D LM_SIGNED_INTERVAL || > ov->list_mode =3D=3D LM_UNSIGNED_INTERVAL); > ov->repeated_opts =3D 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 =3D value; > > - /* see if we're just pushing a list head tracker */ > - if (value =3D=3D NULL) { > - e->is_list_head =3D 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 =3D 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 **lis= tp, > +static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, > Error **errp) > { > - GenericList *list =3D *listp; > - QapiDeallocVisitor *qov =3D to_qov(v); > - StackEntry *e =3D QTAILQ_FIRST(&qov->stack); > - > - if (e && e->is_list_head) { > - e->is_list_head =3D false; > - return list; > - } > - > - if (list) { > - list =3D list->next; > - g_free(*listp); > - return list; > - } > - > - return NULL; > + GenericList *next =3D 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 **err= p) > +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 =3D qiv->stack[qiv->nb_stack - 1].obj; > + StackObject *so =3D &qiv->stack[qiv->nb_stack - 1]; > + QObject *qobj =3D so->obj; > > if (qobj) { > if (name && qobject_type(qobj) =3D=3D QTYPE_QDICT) { > - if (qiv->stack[qiv->nb_stack - 1].h && consume) { > - g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, nam= e); > + if (so->h && consume) { > + g_hash_table_remove(so->h, name); > + } > + qobj =3D qdict_get(qobject_to_qdict(qobj), name); > + } else if (so->entry) { > + qobj =3D qlist_entry_obj(so->entry); > + if (consume) { > + so->entry =3D 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 *q= iv, const char *name, bool consume) { - QObject *qobj =3D qiv->stack[qiv->nb_stack - 1].obj; + StackObject *so =3D &qiv->stack[qiv->nb_stack - 1]; + QObject *qobj =3D so->obj; =20 if (qobj) { if (name && qobject_type(qobj) =3D=3D 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); } } =20 2. Make the actual change @@ -52,9 +52,12 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qi= v, if (so->h && consume) { g_hash_table_remove(so->h, name); } - return qdict_get(qobject_to_qdict(qobj), name); + qobj =3D qdict_get(qobject_to_qdict(qobj), name); } else if (so->entry) { - return qlist_entry_obj(so->entry); + qobj =3D qlist_entry_obj(so->entry); + if (consume) { + so->entry =3D qlist_next(so->entry); + } } } =20 return qobj; =20 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) =3D=3D 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 **e= rrp) > +static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, > + const QListEntry *entry, Error **errp) > { > GHashTable *h; > if (qiv->nb_stack >=3D 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, QObjec= t *obj, Error **errp) > } > > qiv->stack[qiv->nb_stack].obj =3D obj; > - qiv->stack[qiv->nb_stack].entry =3D NULL; > + qiv->stack[qiv->nb_stack].entry =3D entry; > qiv->stack[qiv->nb_stack].h =3D NULL; > > if (qiv->strict && qobject_type(obj) =3D=3D 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 =3D its source QDict, entry =3D 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 **e= rrp) > +static void qmp_input_start_list(Visitor *v, const char *name, > + GenericList **list, Error **errp) > { > QmpInputVisitor *qiv =3D to_qiv(v); > QObject *qobj =3D qmp_input_get_object(qiv, name, true); > + const QListEntry *entry; > > if (!qobj || qobject_type(qobj) !=3D QTYPE_QLIST) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > @@ -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 =3D qlist_first(qobject_to_qlist(qobj)); > + qmp_input_push(qiv, qobj, entry, errp); The stack entry for a list being visited has obj =3D its source QList, entry =3D NULL. > + if (list) { > + if (entry) { > + *list =3D g_new0(GenericList, 1); > + } else { > + *list =3D 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 =3D to_qiv(v); > - GenericList *entry; > StackObject *so =3D &qiv->stack[qiv->nb_stack - 1]; > - bool first; > > - if (so->entry =3D=3D NULL) { > - so->entry =3D qlist_first(qobject_to_qlist(so->obj)); > - first =3D true; > - } else { > - so->entry =3D qlist_next(so->entry); > - first =3D false; > - } > - > - if (so->entry =3D=3D NULL) { > + if (!so->entry) { > return NULL; > } > - > - entry =3D g_malloc0(sizeof(*entry)); > - if (first) { > - *list =3D entry; > - } else { > - (*list)->next =3D entry; > - } > - > - return entry; > + list->next =3D g_new0(GenericList, 1); > + return list->next; > } > > > @@ -375,7 +373,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > v->visitor.optional =3D qmp_input_optional; > v->visitor.get_next_type =3D 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 =3D its source QObject, entry =3D 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 =3D value; > - if (qobject_type(e->value) =3D=3D QTYPE_QLIST) { > - e->is_list_head =3D 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 =3D to_qov(v); > QList *list =3D 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 =3D *listp; > - QmpOutputVisitor *qov =3D to_qov(v); > - QStackEntry *e =3D QTAILQ_FIRST(&qov->stack); > - > - assert(e); > - if (e->is_list_head) { > - e->is_list_head =3D 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 **err= p) > { > StringInputVisitor *siv =3D to_siv(v); > + Error *err =3D 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 *@list =3D NULL on error? Does this handle parse_str() error correctly before your patch? > > siv->cur_range =3D g_list_first(siv->ranges); > if (siv->cur_range) { > @@ -135,14 +141,16 @@ start_list(Visitor *v, const char *name, Error **er= rp) > if (r) { > siv->cur =3D r->begin; > } > + *list =3D g_new0(GenericList, 1); > + } else { > + *list =3D 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 =3D to_siv(v); > - GenericList **link; > Range *r; > > if (!siv->ranges || !siv->cur_range) { > @@ -166,22 +174,13 @@ next_list(Visitor *v, GenericList **list, Error **e= rrp) > siv->cur =3D r->begin; > } > > - if (siv->head) { > - link =3D list; > - siv->head =3D false; > - } else { > - link =3D &(*list)->next; > - } > - > - *link =3D g_malloc0(sizeof **link); > - return *link; > + list->next =3D g_new0(GenericList, 1); > + return list->next; > } > > static void > end_list(Visitor *v) > { > - StringInputVisitor *siv =3D to_siv(v); > - siv->head =3D true; > } > > static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, > @@ -361,6 +360,5 @@ StringInputVisitor *string_input_visitor_new(const ch= ar *str) > v->visitor.optional =3D parse_optional; > > v->string =3D str; > - v->head =3D 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 cha= r *name, double *obj, > } > > static void > -start_list(Visitor *v, const char *name, Error **errp) > +start_list(Visitor *v, const char *name, GenericList **list, Error **err= p) > { > StringOutputVisitor *sov =3D to_sov(v); > > /* we can't traverse a list in a list */ > assert(sov->list_mode =3D=3D LM_NONE); > - sov->list_mode =3D LM_STARTED; > - sov->head =3D 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 =3D 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 =3D to_sov(v); > - GenericList *ret =3D NULL; > - if (*list) { > - if (sov->head) { > - ret =3D *list; > - } else { > - ret =3D (*list)->next; > - } > + GenericList *ret =3D list->next; > > - if (sov->head) { > - if (ret && ret->next =3D=3D NULL) { > - sov->list_mode =3D LM_NONE; > - } > - sov->head =3D false; > - } else { > - if (ret && ret->next =3D=3D NULL) { > - sov->list_mode =3D LM_END; > - } > - } > + if (ret && !ret->next) { > + sov->list_mode =3D 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 =3D=3D LM_NONE || > sov->list_mode =3D=3D LM_IN_PROGRESS); > sov->list_mode =3D LM_NONE; > - sov->head =3D 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 **ob= j, Error **errp) > { > Error *err =3D 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 =3D (GenericList **)obj; > - !err && (i =3D visit_next_list(v, prev, &err)) !=3D NULL; > - prev =3D &i) { > - %(c_name)s *native_i =3D (%(c_name)s *)i; > - visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err); > + elt =3D *obj; > + while (elt) { > + visit_type_%(c_elt_type)s(v, NULL, &elt->value, &err); > + if (err) { > + break; > + } > + elt =3D (%(c_name)s *)visit_next_list(v, (GenericList *)elt, &er= r); > + if (err) { > + break; > + } > } With a simplified next_list(), this could be a nice for-loop: for (tail =3D *obj; tail; tail =3D visit_next_list(v, tail, &err)) .= .. > - > visit_end_list(v); > out: > error_propagate(errp, err);