Eric Blake <ebl...@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 <ebl...@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;
