| Message ID | 1453219845-30939-35-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 (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 <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > 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 *@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 **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 *@list = 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);
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); /* 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. * 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, or set it to + * NULL if there are no elements in the list; and output visitors + * expect *@list to point to the start of the list, if any. 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. * - * 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 + * 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. * * 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 **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. + */ +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) { 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); 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; 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); } -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; } 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); } } @@ -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; @@ -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; @@ -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); + 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); 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) 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; + } 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; } } 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; + } } 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; } @@ -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) 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; - 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; + } } - visit_end_list(v); out: error_propagate(errp, err);
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) 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) } 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 <eblake@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- 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(-)