On 09/12/2015 07:42 AM, Eric Blake wrote: > It looks like the stack is actually tracking two things at once: the > oldest member of the stack (qmp_output_first(), or QTAILQ_LAST) is the > root (can be any QObject), and all other members of the stack hold any > open-ended container QObject (necessarily QDict or QList) that is still > having contents added (the newest member is qmp_output_last(), or > QTAILQ_FIRST). It's a bit confusing that QTAILQ works in the opposite > direction of our terminology. > > Hmm, qmp_output_add_obj() is still a bit odd. If, on a brand new > visitor, we try to visit two scalars in a row (via consecutive > visit_type_int()), the first scalar will become the stack root, then the > second scalar will replace the first as the root. But if we try to > visit two QDict in a row (via consecutive > visit_start_struct(),visit_type_FOO()...,visit_end_struct() sequences), > the first QDict becomes the stack root, gets pushed onto the stack a > second time to be the open-ended container for the visit_type_FOO() > calls, then gets popped only once from the stack when complete. That > means the second QDict will attempt to add itself into the existing > root, instead of replacing the root. A better behavior would be to > blindly replace the root node if the stack has exactly one element (so > that we are consistent on behavior when using a single visitor on two > top-level visits in a row), but that should be a separate patch - in > particular, I don't know how often we ever use a visitor on consecutive > top-level items to need to replace the root (our testsuite allocates a > new visitor every time, instead of reusing visitors). More in another mail.
As in this - instead of abusing the stack to track two things, make it explicit that we have two things (to be applied on top of the previous suggestion, but as a separate patch): diff --git i/qapi/qmp-output-visitor.c w/qapi/qmp-output-visitor.c index cddbdb6..093ffe4 100644 --- i/qapi/qmp-output-visitor.c +++ w/qapi/qmp-output-visitor.c @@ -30,6 +30,7 @@ struct QmpOutputVisitor { Visitor visitor; QStack stack; + QObject *root; }; #define qmp_output_add(qov, name, value) \ @@ -46,6 +47,7 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value) { QStackEntry *e = g_malloc0(sizeof(*e)); + assert(qov->root); assert(value); e->value = value; if (qobject_type(e->value) == QTYPE_QLIST) { @@ -70,23 +72,15 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) /* Grab the root QObject, if any, in preparation to empty the stack */ static QObject *qmp_output_first(QmpOutputVisitor *qov) { - QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); - - if (!e) { - /* No root */ - return NULL; - } - assert(e->value); - return e->value; + return qov->root; } -/* Grab the most recent QObject from the stack, which must exist */ +/* Grab the most recent QObject from the stack, if any */ static QObject *qmp_output_last(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); - assert(e); - return e->value; + return e ? e->value : NULL; } /* Add @value to the current QObject being built. @@ -97,28 +91,23 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, { QObject *cur; - if (QTAILQ_EMPTY(&qov->stack)) { - /* Stack was empty, track this object as root */ - qmp_output_push_obj(qov, value); - return; - } - cur = qmp_output_last(qov); - switch (qobject_type(cur)) { - case QTYPE_QDICT: - assert(name); - qdict_put_obj(qobject_to_qdict(cur), name, value); - break; - case QTYPE_QLIST: - qlist_append_obj(qobject_to_qlist(cur), value); - break; - default: - /* The previous root was a scalar, replace it with a new root */ - qobject_decref(qmp_output_pop(qov)); - assert(QTAILQ_EMPTY(&qov->stack)); - qmp_output_push_obj(qov, value); - break; + if (!cur) { + qobject_decref(qov->root); + qov->root = value; + } else { + switch (qobject_type(cur)) { + case QTYPE_QDICT: + assert(name); + qdict_put_obj(qobject_to_qdict(cur), name, value); + break; + case QTYPE_QLIST: + qlist_append_obj(qobject_to_qlist(cur), value); + break; + default: + g_assert_not_reached(); + } } } @@ -223,16 +212,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v) { QStackEntry *e, *tmp; - /* The bottom QStackEntry, if any, owns the root QObject. See the - * qmp_output_push_obj() invocations in qmp_output_add_obj(). */ - QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v); - QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) { QTAILQ_REMOVE(&v->stack, e, node); g_free(e); } - qobject_decref(root); + qobject_decref(v->root); g_free(v); } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature