Message ID | 1460131992-32278-18-git-send-email-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > Don't embed the root of the visit into the stack of current > containers being visited. That way, we no longer get confused I got confused pretty much every time I looked at this code... > on whether the first visit of a dictionary is to the dictionary > itself or to one of the members of the dictionary, and we no > longer have to require the root visit to pass name=NULL. > > An audit of all qmp_input_visitor_new* call sites shows that > the only places where the visit starts directly on a QDict, > but where the first visit_type_* within the visit had passed > a non-NULL name, were fixed in the previous two places to > properly push into the object with visit_start_struct(). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v14: no change > v13: no change > v12: new patch > --- > include/qapi/visitor.h | 3 +-- > include/qapi/qmp-input-visitor.h | 8 ------ > qapi/qmp-input-visitor.c | 54 ++++++++++++++++++++++------------------ > 3 files changed, 31 insertions(+), 34 deletions(-) > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index 4c29722..e1213a3 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -47,8 +47,7 @@ > * > * The @name parameter of visit_type_FOO() describes the relation > * between this QAPI value and its parent container. When visiting > - * the root of a tree, @name is usually ignored (although some > - * visitors require it to be NULL); when visiting a member of an > + * the root of a tree, @name is ignored; when visiting a member of an > * object, @name is the key associated with the value; and when > * visiting a member of a list, @name is NULL. > * Should we require a root visit's name to be null? Not in this patch. > diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h > index d75ff98..3ed499c 100644 > --- a/include/qapi/qmp-input-visitor.h > +++ b/include/qapi/qmp-input-visitor.h > @@ -19,14 +19,6 @@ > > typedef struct QmpInputVisitor QmpInputVisitor; > > -/* > - * FIXME: When visiting a QDict, passing a non-NULL @name for the > - * first visit_type_FOO() when the root is a QDict will find that > - * particular key within the QDict. In the future, the contract may > - * be tightened to require visit_start_struct() with ignored @name as > - * the first visit; in the meantime, the first visit is safest when > - * using NULL for @name. > - */ > QmpInputVisitor *qmp_input_visitor_new(QObject *obj); > QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj); > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index a94cfa9..16f2f5a 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -39,9 +39,11 @@ struct QmpInputVisitor > { > Visitor visitor; > > - /* Stack of objects being visited. stack[0] is root of visit, > - * stack[1] and below correspond to visit_start_struct (nested > - * QDict) and visit_start_list (nested QList). */ > + /* Root of visit at visitor creation. */ > + QObject *root; > + > + /* Stack of objects being visited (all entries will be either > + * QDict or QList). */ > StackObject stack[QIV_STACK_SIZE]; > int nb_stack; > > @@ -58,36 +60,40 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > const char *name, > bool consume) > { > - StackObject *tos = &qiv->stack[qiv->nb_stack - 1]; > - QObject *qobj = tos->obj; > + StackObject *tos; > + QObject *qobj; > > + if (!qiv->nb_stack) { > + /* Starting at root, name is ignored. */ > + return qiv->root; > + } > + > + /* We are in a container; find the next element */ > + tos = &qiv->stack[qiv->nb_stack - 1]; > + qobj = tos->obj; > assert(qobj); > > - /* If we have a name, and we're in a dictionary, then return that > - * value. */ > - if (name && qobject_type(qobj) == QTYPE_QDICT) { > + if (qobject_type(qobj) == QTYPE_QDICT) { > + assert(name); > qobj = qdict_get(qobject_to_qdict(qobj), name); > if (tos->h && consume && qobj) { > bool removed = g_hash_table_remove(tos->h, name); > assert(removed); > } > - return qobj; > - } > - > - /* If we are in the middle of a list, then return the next element > - * of the list. */ > - if (tos->entry) { > + } else { > assert(qobject_type(qobj) == QTYPE_QLIST); > - assert(!tos->first); > - qobj = qlist_entry_obj(tos->entry); > - if (consume) { > - tos->entry = qlist_next(tos->entry); > + /* FIXME: assertion needs adjustment if we fix visit-core > + * to pass "name.0" style name during lists. */ My remarks to the same comment in PATCH 13 apply. Additionally, if we want a comment on name here, is this the correct patch to add it? > + assert(!name); > + > + if (tos->entry) { > + assert(!tos->first); > + qobj = qlist_entry_obj(tos->entry); > + if (consume) { > + tos->entry = qlist_next(tos->entry); > + } > } > - return qobj; > } > - > - /* Otherwise, we are at the root of the visit or the start of a > - * list, and return the object as-is. */ > return qobj; > } > qobj has two roles in this function. First, it's tos->obj. Then it morphs into the function result, which is either the member of tos->obj named by name, the proper tail of tos->obj pointed to by tos->entry, or tos->obj itself. I'd prefer to keep them separate, perhaps like this: /* We are in a container; find the next element */ tos = &qiv->stack[qiv->nb_stack - 1]; assert(tos->qobj); if (qobject_type(tos->qobj) == QTYPE_QDICT) { assert(name); qobj = qdict_get(qobject_to_qdict(tos->qobj), name); if (tos->h && consume && qobj) { bool removed = g_hash_table_remove(tos->h, name); assert(removed); } } else { assert(qobject_type(tos->qobj) == QTYPE_QLIST); assert(!name); if (tos->entry) { assert(!tos->first); qobj = qlist_entry_obj(tos->entry); if (consume) { tos->entry = qlist_next(tos->entry); } } else { qobj = tos->qobj; } } return qobj; Having a second variable to hold tos->qobj would fine, too. Something else that's bugging me: we step tos->entry here only for the "not first" case. For the "first" case, we do it in qmp_input_push(). Could we clean that up? Not necessarily in this patch or even series. > @@ -373,7 +379,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v) > > void qmp_input_visitor_cleanup(QmpInputVisitor *v) > { > - qobject_decref(v->stack[0].obj); > + qobject_decref(v->root); > g_free(v); > } > > @@ -400,7 +406,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > v->visitor.type_null = qmp_input_type_null; > v->visitor.optional = qmp_input_optional; > > - qmp_input_push(v, obj, NULL, NULL); > + v->root = obj; > qobject_incref(obj); > > return v;
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 4c29722..e1213a3 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -47,8 +47,7 @@ * * The @name parameter of visit_type_FOO() describes the relation * between this QAPI value and its parent container. When visiting - * the root of a tree, @name is usually ignored (although some - * visitors require it to be NULL); when visiting a member of an + * the root of a tree, @name is ignored; when visiting a member of an * object, @name is the key associated with the value; and when * visiting a member of a list, @name is NULL. * diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h index d75ff98..3ed499c 100644 --- a/include/qapi/qmp-input-visitor.h +++ b/include/qapi/qmp-input-visitor.h @@ -19,14 +19,6 @@ typedef struct QmpInputVisitor QmpInputVisitor; -/* - * FIXME: When visiting a QDict, passing a non-NULL @name for the - * first visit_type_FOO() when the root is a QDict will find that - * particular key within the QDict. In the future, the contract may - * be tightened to require visit_start_struct() with ignored @name as - * the first visit; in the meantime, the first visit is safest when - * using NULL for @name. - */ QmpInputVisitor *qmp_input_visitor_new(QObject *obj); QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj); diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index a94cfa9..16f2f5a 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -39,9 +39,11 @@ struct QmpInputVisitor { Visitor visitor; - /* Stack of objects being visited. stack[0] is root of visit, - * stack[1] and below correspond to visit_start_struct (nested - * QDict) and visit_start_list (nested QList). */ + /* Root of visit at visitor creation. */ + QObject *root; + + /* Stack of objects being visited (all entries will be either + * QDict or QList). */ StackObject stack[QIV_STACK_SIZE]; int nb_stack; @@ -58,36 +60,40 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv, const char *name, bool consume) { - StackObject *tos = &qiv->stack[qiv->nb_stack - 1]; - QObject *qobj = tos->obj; + StackObject *tos; + QObject *qobj; + if (!qiv->nb_stack) { + /* Starting at root, name is ignored. */ + return qiv->root; + } + + /* We are in a container; find the next element */ + tos = &qiv->stack[qiv->nb_stack - 1]; + qobj = tos->obj; assert(qobj); - /* If we have a name, and we're in a dictionary, then return that - * value. */ - if (name && qobject_type(qobj) == QTYPE_QDICT) { + if (qobject_type(qobj) == QTYPE_QDICT) { + assert(name); qobj = qdict_get(qobject_to_qdict(qobj), name); if (tos->h && consume && qobj) { bool removed = g_hash_table_remove(tos->h, name); assert(removed); } - return qobj; - } - - /* If we are in the middle of a list, then return the next element - * of the list. */ - if (tos->entry) { + } else { assert(qobject_type(qobj) == QTYPE_QLIST); - assert(!tos->first); - qobj = qlist_entry_obj(tos->entry); - if (consume) { - tos->entry = qlist_next(tos->entry); + /* FIXME: assertion needs adjustment if we fix visit-core + * to pass "name.0" style name during lists. */ + assert(!name); + + if (tos->entry) { + assert(!tos->first); + qobj = qlist_entry_obj(tos->entry); + if (consume) { + tos->entry = qlist_next(tos->entry); + } } - return qobj; } - - /* Otherwise, we are at the root of the visit or the start of a - * list, and return the object as-is. */ return qobj; } @@ -373,7 +379,7 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v) void qmp_input_visitor_cleanup(QmpInputVisitor *v) { - qobject_decref(v->stack[0].obj); + qobject_decref(v->root); g_free(v); } @@ -400,7 +406,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) v->visitor.type_null = qmp_input_type_null; v->visitor.optional = qmp_input_optional; - qmp_input_push(v, obj, NULL, NULL); + v->root = obj; qobject_incref(obj); return v;
Don't embed the root of the visit into the stack of current containers being visited. That way, we no longer get confused on whether the first visit of a dictionary is to the dictionary itself or to one of the members of the dictionary, and we no longer have to require the root visit to pass name=NULL. An audit of all qmp_input_visitor_new* call sites shows that the only places where the visit starts directly on a QDict, but where the first visit_type_* within the visit had passed a non-NULL name, were fixed in the previous two places to properly push into the object with visit_start_struct(). Signed-off-by: Eric Blake <eblake@redhat.com> --- v14: no change v13: no change v12: new patch --- include/qapi/visitor.h | 3 +-- include/qapi/qmp-input-visitor.h | 8 ------ qapi/qmp-input-visitor.c | 54 ++++++++++++++++++++++------------------ 3 files changed, 31 insertions(+), 34 deletions(-)