Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5258 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]