@@ -166,7 +166,7 @@
* if (err) {
* goto outobj;
* }
- * visit_start_list(v, "list", &err);
+ * visit_start_list(v, "list", NULL, 0, &err);
* if (err) {
* goto outobj;
* }
@@ -268,39 +268,43 @@ void visit_end_struct(Visitor *v);
* @name expresses the relationship of this list to its parent
* container; see the general description of @name above.
*
+ * @list must be non-NULL for a real walk, in which case @size
+ * determines how much memory an input visitor will allocate into
+ * *@list (at least sizeof(GenericList)). Some visitors also allow
+ * @list to be NULL for a virtual walk, in which case @size is
+ * ignored.
+ *
* @errp must be NULL-initialized, and is set if an error is detected
* (such as if a member @name is not present, or is present but not a
- * list).
+ * list). On error, input visitors set *@obj to NULL.
*
* After visit_start_list() succeeds, the caller may visit its members
- * one after the other. A real visit uses visit_next_list() for
- * traversing the linked list, while a virtual visit uses other means.
- * For each list element, call the appropriate visit_type_FOO() with
- * name set to NULL and obj set to the address of the list element.
- * Finally, visit_end_list() needs to be called to clean up. See the
- * examples above.
+ * one after the other. A real visit (where @obj was non-NULL) uses
+ * visit_next_list() for traversing the linked list, while a virtual
+ * visit (where @obj was NULL) uses other means. For each list
+ * element, call the appropriate visit_type_FOO() with name set to
+ * NULL and obj set to the address of the list element. Finally,
+ * visit_end_list() needs to be called to clean up. See the examples
+ * above.
*/
-void visit_start_list(Visitor *v, const char *name, Error **errp);
+void visit_start_list(Visitor *v, const char *name, GenericList **list,
+ size_t size, Error **errp);
/*
- * Iterate over a GenericList during a list visit.
+ * Iterate over a GenericList during a non-virtual list visit.
*
- * @size represents the size of a linked list node.
+ * @size represents the size of a linked list node (at least
+ * sizeof(GenericList)).
*
- * @list must not be NULL; on the first call, @list contains the
- * address of the list head, and on subsequent calls *@list must be
- * the previously returned value. Must be called in a loop until a
+ * @tail must not be NULL; on the first call, @tail is the value of
+ * *list after visit_start_list(), and on subsequent calls @tail must
+ * be the previously returned value. Must be called in a loop until a
* NULL return or error occurs; for each non-NULL return, the caller
* must then call the appropriate visit_type_*() for the element type
* of the list, with that function's name parameter set to NULL and
- * obj set to the address of (*@list)->value.
- *
- * FIXME: This interface is awkward; it requires all callbacks to
- * track whether it is the first or a subsequent call. A better
- * interface would pass the head of the list through
- * visit_start_list().
+ * obj set to the address of @tail->value.
*/
-GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
+GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
/*
* Complete a list visit started earlier.
@@ -45,11 +45,12 @@ struct Visitor
/* Must be set to visit structs. */
void (*end_struct)(Visitor *v);
- /* Must be set. */
- void (*start_list)(Visitor *v, const char *name, Error **errp);
+ /* Must be set; document if @list may not be NULL. */
+ void (*start_list)(Visitor *v, const char *name, GenericList **list,
+ size_t size, Error **errp);
/* Must be set. */
- GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
+ GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
/* Must be set. */
void (*end_list)(Visitor *v);
@@ -119,20 +119,20 @@ def gen_visit_list(name, element_type):
void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
{
Error *err = NULL;
- GenericList *i, **prev;
+ %(c_name)s *tail;
+ size_t size = sizeof(**obj);
- visit_start_list(v, name, &err);
+ visit_start_list(v, name, (GenericList **)obj, size, &err);
if (err) {
goto out;
}
-
- for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
- prev = &i) {
- %(c_name)s *native_i = (%(c_name)s *)i;
- visit_type_%(c_elt_type)s(v, NULL, &native_i->value, &err);
+ for (tail = *obj; tail;
+ tail = (%(c_name)s *)visit_next_list(v, (GenericList *)tail, size)) {
+ visit_type_%(c_elt_type)s(v, NULL, &tail->value, &err);
+ if (err) {
+ break;
+ }
}
-
visit_end_list(v);
out:
error_propagate(errp, err);
@@ -32,7 +32,7 @@ typedef struct OptsVisitor OptsVisitor;
*
* The Opts input visitor does not yet implement support for visiting
* QAPI alternates, numbers (other than integers), null, or arbitrary
- * QTypes.
+ * QTypes. It also requires non-NULL list to visit_start_list().
*/
OptsVisitor *opts_visitor_new(const QemuOpts *opts);
void opts_visitor_cleanup(OptsVisitor *nv);
@@ -19,7 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
/*
* The string input visitor does not yet implement support for
- * visiting QAPI structs, alternates, null, or arbitrary QTypes.
+ * visiting QAPI structs, alternates, null, or arbitrary QTypes. It
+ * also requires non-NULL list to visit_start_list().
*/
StringInputVisitor *string_input_visitor_new(const char *str);
void string_input_visitor_cleanup(StringInputVisitor *v);
@@ -19,7 +19,8 @@ typedef struct StringOutputVisitor StringOutputVisitor;
/*
* The string output visitor does not yet implement support for
- * visiting QAPI structs, alternates, null, or arbitrary QTypes.
+ * visiting QAPI structs, alternates, null, or arbitrary QTypes. It
+ * also requires non-NULL list to visit_start_list().
*/
StringOutputVisitor *string_output_visitor_new(bool human);
void string_output_visitor_cleanup(StringOutputVisitor *v);
@@ -41,15 +41,17 @@ void visit_end_struct(Visitor *v)
v->end_struct(v);
}
-void visit_start_list(Visitor *v, const char *name, Error **errp)
+void visit_start_list(Visitor *v, const char *name, GenericList **list,
+ size_t size, Error **errp)
{
- v->start_list(v, name, errp);
+ assert(!list || size >= sizeof(GenericList));
+ v->start_list(v, name, list, size, errp);
}
-GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
+GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
{
- assert(list && size >= sizeof(GenericList));
- return v->next_list(v, list, size);
+ assert(tail && size >= sizeof(GenericList));
+ return v->next_list(v, tail, size);
}
void visit_end_list(Visitor *v)
@@ -300,7 +300,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
int i;
prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
- visit_start_list(v, name, &err);
+ visit_start_list(v, name, NULL, 0, &err);
if (err) {
error_propagate(errp, err);
return;
@@ -22,9 +22,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
@@ -213,35 +212,33 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
static void
-opts_start_list(Visitor *v, const char *name, Error **errp)
+opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+ Error **errp)
{
OptsVisitor *ov = to_ov(v);
/* we can't traverse a list in a list */
assert(ov->list_mode == LM_NONE);
+ /* we don't support visits without GenericList, yet */
+ assert(list);
ov->repeated_opts = lookup_distinct(ov, name, errp);
- if (ov->repeated_opts != NULL) {
- ov->list_mode = LM_STARTED;
+ if (ov->repeated_opts) {
+ ov->list_mode = LM_IN_PROGRESS;
+ *list = g_malloc0(size);
+ } else {
+ *list = NULL;
}
}
static GenericList *
-opts_next_list(Visitor *v, GenericList **list, size_t size)
+opts_next_list(Visitor *v, GenericList *tail, size_t size)
{
OptsVisitor *ov = to_ov(v);
- GenericList **link;
switch (ov->list_mode) {
- case LM_STARTED:
- ov->list_mode = LM_IN_PROGRESS;
- link = list;
- break;
-
case LM_SIGNED_INTERVAL:
case LM_UNSIGNED_INTERVAL:
- link = &(*list)->next;
-
if (ov->list_mode == LM_SIGNED_INTERVAL) {
if (ov->range_next.s < ov->range_limit.s) {
++ov->range_next.s;
@@ -262,7 +259,6 @@ opts_next_list(Visitor *v, GenericList **list, size_t size)
g_hash_table_remove(ov->unprocessed_opts, opt->name);
return NULL;
}
- link = &(*list)->next;
break;
}
@@ -270,8 +266,8 @@ opts_next_list(Visitor *v, GenericList **list, size_t size)
abort();
}
- *link = g_malloc0(size);
- return *link;
+ tail->next = g_malloc0(size);
+ return tail->next;
}
@@ -280,8 +276,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;
@@ -22,7 +22,6 @@
typedef struct StackEntry
{
void *value;
- bool is_list_head;
QTAILQ_ENTRY(StackEntry) node;
} StackEntry;
@@ -43,10 +42,6 @@ static void qapi_dealloc_push(QapiDeallocVisitor *qov, void *value)
e->value = value;
- /* see if we're just pushing a list head tracker */
- if (value == NULL) {
- e->is_list_head = true;
- }
QTAILQ_INSERT_HEAD(&qov->stack, e, node);
}
@@ -93,38 +88,22 @@ static void qapi_dealloc_end_alternate(Visitor *v)
}
}
-static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp)
+static void qapi_dealloc_start_list(Visitor *v, const char *name,
+ GenericList **list, size_t size,
+ Error **errp)
{
- QapiDeallocVisitor *qov = to_qov(v);
- qapi_dealloc_push(qov, NULL);
}
-static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
+static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *tail,
size_t size)
{
- GenericList *list = *listp;
- QapiDeallocVisitor *qov = to_qov(v);
- StackEntry *e = QTAILQ_FIRST(&qov->stack);
-
- if (e && e->is_list_head) {
- e->is_list_head = false;
- return list;
- }
-
- if (list) {
- list = list->next;
- g_free(*listp);
- return list;
- }
-
- return NULL;
+ GenericList *next = tail->next;
+ g_free(tail);
+ return next;
}
static void qapi_dealloc_end_list(Visitor *v)
{
- QapiDeallocVisitor *qov = to_qov(v);
- void *obj = qapi_dealloc_pop(qov);
- assert(obj == NULL); /* should've been list head tracker with no payload */
}
static void qapi_dealloc_type_str(Visitor *v, const char *name, char **obj,
@@ -28,8 +28,6 @@ typedef struct StackObject
/* If obj is list: tail of list still needing visits */
const QListEntry *entry;
- /* If obj is list: true if head is not visited yet */
- bool first;
GHashTable *h; /* If obj is dict: remaining keys needing visits */
} StackObject;
@@ -86,7 +84,6 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
assert(!name);
if (tos->entry) {
- assert(!tos->first);
qobj = qlist_entry_obj(tos->entry);
if (consume) {
tos->entry = qlist_next(tos->entry);
@@ -116,7 +113,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
tos->obj = obj;
tos->entry = entry;
- tos->first = true;
tos->h = NULL;
if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
@@ -193,13 +189,17 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
}
-static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
+static void qmp_input_start_list(Visitor *v, const char *name,
+ GenericList **list, size_t size, Error **errp)
{
QmpInputVisitor *qiv = to_qiv(v);
QObject *qobj = qmp_input_get_object(qiv, name, true);
const QListEntry *entry;
if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
+ if (list) {
+ *list = NULL;
+ }
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"list");
return;
@@ -207,28 +207,26 @@ static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
entry = qlist_first(qobject_to_qlist(qobj));
qmp_input_push(qiv, qobj, entry, errp);
+ if (list) {
+ if (entry) {
+ *list = g_malloc0(size);
+ } else {
+ *list = NULL;
+ }
+ }
}
-static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
+static GenericList *qmp_input_next_list(Visitor *v, GenericList *tail,
size_t size)
{
QmpInputVisitor *qiv = to_qiv(v);
- GenericList *entry;
StackObject *so = &qiv->stack[qiv->nb_stack - 1];
if (!so->entry) {
return NULL;
}
-
- entry = g_malloc0(size);
- if (so->first) {
- *list = entry;
- so->first = false;
- } else {
- (*list)->next = entry;
- }
-
- return entry;
+ tail->next = g_malloc0(size);
+ return tail->next;
}
@@ -22,7 +22,6 @@
typedef struct QStackEntry
{
QObject *value;
- bool is_list_head;
QTAILQ_ENTRY(QStackEntry) node;
} QStackEntry;
@@ -52,9 +51,6 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
assert(qov->root);
assert(value);
e->value = value;
- if (qobject_type(e->value) == QTYPE_QLIST) {
- e->is_list_head = true;
- }
QTAILQ_INSERT_HEAD(&qov->stack, e, node);
}
@@ -120,7 +116,9 @@ static void qmp_output_end_struct(Visitor *v)
assert(qobject_type(value) == QTYPE_QDICT);
}
-static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
+static void qmp_output_start_list(Visitor *v, const char *name,
+ GenericList **listp, size_t size,
+ Error **errp)
{
QmpOutputVisitor *qov = to_qov(v);
QList *list = qlist_new();
@@ -129,20 +127,10 @@ static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
qmp_output_push(qov, list);
}
-static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
+static GenericList *qmp_output_next_list(Visitor *v, GenericList *tail,
size_t size)
{
- GenericList *list = *listp;
- QmpOutputVisitor *qov = to_qov(v);
- QStackEntry *e = QTAILQ_FIRST(&qov->stack);
-
- assert(e);
- if (e->is_list_head) {
- e->is_list_head = false;
- return list;
- }
-
- return list ? list->next : NULL;
+ return tail->next;
}
static void qmp_output_end_list(Visitor *v)
@@ -24,8 +24,6 @@ struct StringInputVisitor
{
Visitor visitor;
- bool head;
-
GList *ranges;
GList *cur_range;
int64_t cur;
@@ -124,11 +122,21 @@ error:
}
static void
-start_list(Visitor *v, const char *name, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+ Error **errp)
{
StringInputVisitor *siv = to_siv(v);
+ Error *err = NULL;
- parse_str(siv, errp);
+ /* We don't support visits without a GenericList, yet */
+ assert(list);
+
+ parse_str(siv, &err);
+ if (err) {
+ *list = NULL;
+ error_propagate(errp, err);
+ return;
+ }
siv->cur_range = g_list_first(siv->ranges);
if (siv->cur_range) {
@@ -136,13 +144,15 @@ start_list(Visitor *v, const char *name, Error **errp)
if (r) {
siv->cur = r->begin;
}
+ *list = g_malloc0(size);
+ } else {
+ *list = NULL;
}
}
-static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
+static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
{
StringInputVisitor *siv = to_siv(v);
- GenericList **link;
Range *r;
if (!siv->ranges || !siv->cur_range) {
@@ -166,21 +176,12 @@ static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
siv->cur = r->begin;
}
- if (siv->head) {
- link = list;
- siv->head = false;
- } else {
- link = &(*list)->next;
- }
-
- *link = g_malloc0(size);
- return *link;
+ tail->next = g_malloc0(size);
+ return tail->next;
}
static void end_list(Visitor *v)
{
- StringInputVisitor *siv = to_siv(v);
- siv->head = true;
}
static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
@@ -361,6 +362,5 @@ StringInputVisitor *string_input_visitor_new(const char *str)
v->visitor.optional = parse_optional;
v->string = str;
- v->head = true;
return v;
}
@@ -20,7 +20,7 @@
enum ListMode {
LM_NONE, /* not traversing a list of repeated options */
- LM_STARTED, /* start_list() succeeded */
+ LM_STARTED, /* start_list() succeeded with multiple elements */
LM_IN_PROGRESS, /* next_list() has been called.
*
@@ -48,7 +48,7 @@ enum ListMode {
LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */
- LM_END
+ LM_END, /* next_list() called, about to see last element. */
};
typedef enum ListMode ListMode;
@@ -58,7 +58,6 @@ struct StringOutputVisitor
Visitor visitor;
bool human;
GString *string;
- bool head;
ListMode list_mode;
union {
int64_t s;
@@ -266,39 +265,29 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
}
static void
-start_list(Visitor *v, const char *name, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+ Error **errp)
{
StringOutputVisitor *sov = to_sov(v);
/* we can't traverse a list in a list */
assert(sov->list_mode == LM_NONE);
- sov->list_mode = LM_STARTED;
- sov->head = true;
+ /* We don't support visits without a GenericList, yet */
+ assert(list);
+ /* List handling is only needed if there are at least two elements */
+ if (*list && (*list)->next) {
+ sov->list_mode = LM_STARTED;
+ }
}
-static GenericList *next_list(Visitor *v, GenericList **list, size_t size)
+static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
{
StringOutputVisitor *sov = to_sov(v);
- GenericList *ret = NULL;
- if (*list) {
- if (sov->head) {
- ret = *list;
- } else {
- ret = (*list)->next;
- }
+ GenericList *ret = tail->next;
- if (sov->head) {
- if (ret && ret->next == NULL) {
- sov->list_mode = LM_NONE;
- }
- sov->head = false;
- } else {
- if (ret && ret->next == NULL) {
- sov->list_mode = LM_END;
- }
- }
+ if (ret && !ret->next) {
+ sov->list_mode = LM_END;
}
-
return ret;
}
@@ -311,8 +300,6 @@ static void end_list(Visitor *v)
sov->list_mode == LM_NONE ||
sov->list_mode == LM_IN_PROGRESS);
sov->list_mode = LM_NONE;
- sov->head = true;
-
}
char *string_output_get_string(StringOutputVisitor *sov)
@@ -911,18 +911,19 @@ Example:
void visit_type_UserDefOneList(Visitor *v, const char *name, UserDefOneList **obj, Error **errp)
{
Error *err = NULL;
- GenericList *i, **prev;
+ UserDefOneList *tail;
+ size_t size = sizeof(**obj);
- visit_start_list(v, name, &err);
+ visit_start_list(v, name, (GenericList **)obj, size, &err);
if (err) {
goto out;
}
-
- for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
- prev = &i) {
- UserDefOneList *native_i = (UserDefOneList *)i;
- visit_type_UserDefOne(v, NULL, &native_i->value, &err);
+ for (tail = *obj; tail;
+ tail = (UserDefOneList *)visit_next_list(v, (GenericList *)tail, size)) {
+ visit_type_UserDefOne(v, NULL, &tail->value, &err);
+ if (err) {
+ break;
+ }
}
visit_end_list(v);
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() for (prev = head; cur = next(prev); prev = &cur) { visit(&cur->value) } Note that these semantics (advance before visit) requires that the first call to next() return the list head, while all other calls return the next element of the list; that is, every visitor implementation is required to track extra state to decide whether to return the input as-is, or to advance. It also requires an argument of 'GenericList **' to next(), solely because the first iteration might need to modify the caller's GenericList head, so that all other calls have to do a layer of dereferencing. We can greatly simplify things by hoisting the special case into the start() routine, and flipping the order in the loop to visit before advance: start(head) for (tail = *head; tail; tail = next(tail)) { visit(&tail->value) } With the simpler semantics, visitors have less state to track, the argument to next() is reduced to 'GenericList *', and it also becomes obvious whether an input visitor is allocating a FooList during visit_start_list() (rather than the old way of not knowing if an allocation happened until the first visit_next_list()). The signature of visit_start_list() is chosen to match visit_start_struct(), with the new parameters after 'name'. The spapr_drc case is a virtual visit, done by passing NULL for list, similarly to how NULL is passed to visit_start_struct() when a qapi type is not used in those visits. It was easy to provide these semantics for qmp-output and dealloc visitors, and a bit harder for qmp-input (several prerequisite patches refactored things to make this patch straightforward). But it turned out that the string and opts visitors munge enough other state during visit_next_list() to make it easier to just document and require a GenericList visit for now; an assertion will remind us to adjust things if we need the semantics in the future. Signed-off-by: Eric Blake <eblake@redhat.com> --- v12: rebase to earlier changes, drop R-b, improve documentation, split out qmp-input cleanups into prereq patches [no v10, v11] v9: no change v8: consistent parameter order, fix qmp_input_get_next_type() to not skip list entries v7: new patch --- include/qapi/visitor.h | 46 ++++++++++++++++++++---------------- include/qapi/visitor-impl.h | 7 +++--- scripts/qapi-visit.py | 18 +++++++------- include/qapi/opts-visitor.h | 2 +- include/qapi/string-input-visitor.h | 3 ++- include/qapi/string-output-visitor.h | 3 ++- qapi/qapi-visit-core.c | 12 ++++++---- hw/ppc/spapr_drc.c | 2 +- qapi/opts-visitor.c | 33 +++++++++++--------------- qapi/qapi-dealloc-visitor.c | 35 ++++++--------------------- qapi/qmp-input-visitor.c | 32 ++++++++++++------------- qapi/qmp-output-visitor.c | 22 ++++------------- qapi/string-input-visitor.c | 36 ++++++++++++++-------------- qapi/string-output-visitor.c | 41 +++++++++++--------------------- docs/qapi-code-gen.txt | 17 ++++++------- 15 files changed, 133 insertions(+), 176 deletions(-)