From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48371) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a69NT-0004kA-BU for qemu-devel@nongnu.org; Mon, 07 Dec 2015 22:55:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a69NQ-0003gF-OL for qemu-devel@nongnu.org; Mon, 07 Dec 2015 22:55:47 -0500 From: Eric Blake Date: Mon, 7 Dec 2015 20:55:19 -0700 Message-Id: <1449546921-6378-30-git-send-email-eblake@redhat.com> In-Reply-To: <1449546921-6378-1-git-send-email-eblake@redhat.com> References: <1449546921-6378-1-git-send-email-eblake@redhat.com> Subject: [Qemu-devel] [PATCH v7 29/31] qapi: Simplify semantics of visit_next_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Michael Roth , Alexander Graf , "open list:sPAPR pseries" , armbru@redhat.com, David Gibson 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 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 --- v7: new patch --- hw/ppc/spapr_drc.c | 2 +- include/qapi/visitor-impl.h | 5 ++-- include/qapi/visitor.h | 44 +++++++++++++++++-------------- qapi/opts-visitor.c | 32 ++++++++++------------- qapi/qapi-dealloc-visitor.c | 29 +++++---------------- qapi/qapi-visit-core.c | 7 ++--- qapi/qmp-input-visitor.c | 62 +++++++++++++++++++++----------------------- 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, 123 insertions(+), 170 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 65764bc..f5ea3e0 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, void *opaque, 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 d1f4f78..9654be6 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 f83707a..3b9f429 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -111,32 +111,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 fabf3d7..ae39531 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 d3f9171..fc66dad 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 a3b59f7..a7e6ca8 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 18c963b..20db105 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -44,16 +44,18 @@ 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); + qobj = qdict_get(qobject_to_qdict(qobj), name); + } else if (so->entry) { + qobj = qlist_entry_obj(so->entry); + so->entry = qlist_next(so->entry); } } @@ -66,7 +68,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 +79,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 +141,7 @@ static void qmp_input_start_struct(Visitor *v, void **obj, size_t size, return; } - qmp_input_push(qiv, qobj, &err); + qmp_input_push(qiv, qobj, NULL, &err); if (err) { error_propagate(errp, err); return; @@ -158,10 +161,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 +174,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; } @@ -361,7 +357,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 1ffe820..aa07f27 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); } @@ -123,7 +119,8 @@ static void qmp_output_end_struct(Visitor *v) qmp_output_pop(qov); } -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(); @@ -132,20 +129,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 0b0bb6e..0adbe79 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, int64_t *obj, const char *name, @@ -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 13b00c7..1410d9b 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, double *obj, const char *name, } 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 694951f..6d4f6ba 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, %(c_name)s **obj, const char *name, 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, &native_i->value, NULL, &err); + elt = *obj; + while (elt) { + visit_type_%(c_elt_type)s(v, &elt->value, NULL, &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); -- 2.4.3