Eric Blake <ebl...@redhat.com> writes: > In the QMP input visitor, visiting a list traverses two objects: > the QAPI GenericList of the caller (which gets advanced in > visit_next_list() regardless of this patch), and the QList input > that we are converting to QAPI. For consistency with QDict > visits, we want to consume elements from the input QList during > the visit_type_FOO() for the list element; that is, we want ALL > the code for consuming an input to live in qmp_input_get_object(), > rather than having it split according to whether we are visiting > a dict or a list. Making qmp_input_get_object() the common point > of consumption will make it easier for a later patch to refactor > visit_start_list() to cover the GenericList * head of a QAPI list, > and in turn will get rid of the 'first' flag (which lived in > qmp_input_next_list() pre-patch, and is hoisted to StackObject > by this patch). > > This patch is therefore shifting the post-condition use of > 'entry' from: > > start_list next_list type_ELT ... next_list type_ELT end_list > entry NULL 1st elt 1st elt last elt last elt gone > > where type_ELT() returns (entry ? entry : 1st elt) and next_list() steps > entry > > to this usage: > > start_list next_list type_ELT ... next_list type_ELT end_list > entry 1st elt 1nd elt 2nd elt last elt NULL gone > > where type_ELT() steps entry and returns the old entry, and next_list() > leaves entry alone. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v15: rebase, improve commit message, hoist root handling earlier > so that this patch is more pinpointed > v14: no change > v13: no change > v12: new patch > --- > qapi/qmp-input-visitor.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 6409a83..eb77adc 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -29,6 +29,7 @@ typedef struct StackObject > > GHashTable *h; /* If obj is dict: unvisited keys */ > const QListEntry *entry; /* If obj is list: unvisited tail */ > + bool first; /* If obj is list: will next_list() visit head > */
I like documenting bools with the yes/no question they answer. Questions end with a question mark, though :) Perhaps: /* If obj is list: next_list() not yet called? */ Since this will go away soon, feel free not to change anything here. > } StackObject; > > struct QmpInputVisitor > @@ -80,7 +81,11 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > } else { > assert(qobject_type(qobj) == QTYPE_QLIST); > assert(!name); > + assert(!tos->first); > ret = qlist_entry_obj(tos->entry); > + if (consume) { > + tos->entry = qlist_next(tos->entry); > + } > } > > return ret; > @@ -104,13 +109,16 @@ static void qmp_input_push(QmpInputVisitor *qiv, > QObject *obj, Error **errp) > } > > tos->obj = obj; > - tos->entry = NULL; > - tos->h = NULL; > + assert(!tos->h); > + assert(!tos->entry); > > if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) { > h = g_hash_table_new(g_str_hash, g_str_equal); > qdict_iter(qobject_to_qdict(obj), qdict_add_key, h); > tos->h = h; > + } else if (qobject_type(obj) == QTYPE_QLIST) { > + tos->entry = qlist_first(qobject_to_qlist(obj)); > + tos->first = true; > } > > qiv->nb_stack++; > @@ -119,10 +127,11 @@ static void qmp_input_push(QmpInputVisitor *qiv, > QObject *obj, Error **errp) > > static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) > { > + StackObject *tos = &qiv->stack[qiv->nb_stack - 1]; > assert(qiv->nb_stack > 0); > > if (qiv->strict) { > - GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h; > + GHashTable * const top_ht = tos->h; Let's clean this up to *const while we're touching it. Can do on commit. > if (top_ht) { > GHashTableIter iter; > const char *key; > @@ -133,6 +142,7 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error > **errp) > } > g_hash_table_unref(top_ht); > } > + tos->h = NULL; > } > > qiv->nb_stack--; > @@ -192,23 +202,15 @@ static GenericList *qmp_input_next_list(Visitor *v, > GenericList **list, > QmpInputVisitor *qiv = to_qiv(v); > GenericList *entry; > StackObject *so = &qiv->stack[qiv->nb_stack - 1]; > - bool first; > > - if (so->entry == NULL) { > - so->entry = qlist_first(qobject_to_qlist(so->obj)); > - first = true; > - } else { > - so->entry = qlist_next(so->entry); > - first = false; > - } > - > - if (so->entry == NULL) { > + if (!so->entry) { > return NULL; > } > > entry = g_malloc0(size); > - if (first) { > + if (so->first) { > *list = entry; > + so->first = false; > } else { > (*list)->next = entry; > } This stuff never fails to confuse me. But now there's hope it's mostly because the *old* code is confusing.