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: 8039 bytes --]

On 09/12/2015 06:16 AM, Eric Blake wrote:

>>
>> where e = QTAILQ_LAST(&qov->stack, QStack)
>>
>> Questions:
>>
>> * How can e become NULL?
>>
>>   The only place that pushes onto &qov->stack appears to be
>>   qmp_output_push_obj().  Can obviously push e with !e->value, but can't
>>   push null e.
> 
> My understanding is that qov->stack corresponds to nesting levels of {}
> or [] in the JSON code.  The testsuite shows a case where the stack is
> empty as one case where e is NULL, but if e is non-NULL, I'm not sure
> whether e->value can ever be NULL.  I'll have to read the code more closely.

I agree that qmp_object_push_obj() is the only place that pushes; and it
promptly calls qobject_type(e->value) which blindly dereferences
value->type.  So it sounds like e->value can never be NULL (or we would
segfault), although a well-placed assert in qmp-output-visitor.c would
be nicer than having to chase through qobject.h.

So the only way for qmp_object_first() to return NULL is if e is NULL
(the stack was empty), since e->value would be non-NULL.  Your patch
fixed it to never return NULL, mine fixed it to handle a NULL return in
callers that care (only 1 of the 2 callers).

> 
>>
>> * What about qmp_output_last()?
>>
>>   Why does qmp_output_first() check for !e, but not qmp_output_last()?
>>
>>   My patch makes them less symmetric (bad smell).  Yours makes them more
>>   symmetric, but not quite.
> 
> Which is awkward in its own right.

qmp_output_last() is only used by qmp_output_add_obj(), which first
checks for an empty queue; so the queue cannot be empty.  Therefore,
qmp_output_last() could assert that e != NULL. At any rate,
qmp_output_last() never returns NULL; pre-patch, only qmp_output_first()
could do so.

And qmp_output_add_obj() blindly calls qobject_type() on the result of
qmp_output_last(), which as argued before would segfault if e->value
could ever be NULL.

It looks like the role of qmp_output_last() is to determine the last
thing that was being output; if it is a QDict or QList, then add the
current visit on to that existing object; otherwise, the last thing
output is a scalar and is complete, so we are replacing the root with
the new object being output.  Meanwhile, the role of qmp_output_first()
appears to be to grab the root of the output tree - either to give the
caller the overall QObject* created by visiting a structure
(qmp_output_get_qobject, which must not return NULL), or to clean up all
references still stored in the stack.  It also looks like
qmp_output_get_qobject() can be called multiple times before cleanup (to
grab copies of the current root).

> 
>>
>> * How does this contraption work?
> 
> Indeed. Without reading further, we're both shooting in the dark for
> something that makes tests pass, but without being a clean interface.

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.

For the sake of our current issue, I think that adding comments to the
existing behavior is sufficient to explain why my approach works.  So
how about squashing this:


diff --git c/qapi/qmp-output-visitor.c w/qapi/qmp-output-visitor.c
index 2d6083e..d42ca0e 100644
--- c/qapi/qmp-output-visitor.c
+++ w/qapi/qmp-output-visitor.c
@@ -41,10 +41,12 @@ static QmpOutputVisitor *to_qov(Visitor *v)
     return container_of(v, QmpOutputVisitor, visitor);
 }

+/* Push @value onto the stack of current QObjects being built */
 static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value)
 {
     QStackEntry *e = g_malloc0(sizeof(*e));

+    assert(value);
     e->value = value;
     if (qobject_type(e->value) == QTYPE_QLIST) {
         e->is_list_head = true;
@@ -52,39 +54,51 @@ static void qmp_output_push_obj(QmpOutputVisitor
*qov, QObject *value)
     QTAILQ_INSERT_HEAD(&qov->stack, e, node);
 }

+/* Grab and remove the most recent QObject from the stack */
 static QObject *qmp_output_pop(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);
     QObject *value;
+
+    assert(e);
     QTAILQ_REMOVE(&qov->stack, e, node);
     value = e->value;
     g_free(e);
     return value;
 }

+/* 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) {
-        return qnull();
+        /* No root */
+        return NULL;
     }
-
+    assert (e->value);
     return e->value;
 }

+/* Grab the most recent QObject from the stack, which must exist */
 static QObject *qmp_output_last(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);
+
+    assert(e);
     return e->value;
 }

+/* Add @value to the current QObject being built.
+ * If the stack is visiting a dictionary or list, @value is now owned
+ * by that container. Otherwise, @value is now the root.  */
 static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
                                QObject *value)
 {
     QObject *cur;

     if (QTAILQ_EMPTY(&qov->stack)) {
+        /* Stack was empty, track this object as root */
         qmp_output_push_obj(qov, value);
         return;
     }
@@ -93,13 +107,16 @@ static void qmp_output_add_obj(QmpOutputVisitor
*qov, const char *name,

     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;
     }
@@ -185,11 +202,14 @@ static void qmp_output_type_number(Visitor *v,
double *obj, const char *name,
     qmp_output_add(qov, name, qfloat_from_double(*obj));
 }

+/* Finish building, and return the root object. Will not be NULL. */
 QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
 {
     QObject *obj = qmp_output_first(qov);
     if (obj) {
         qobject_incref(obj);
+    } else {
+        obj = qnull();
     }
     return obj;
 }


-- 
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 --]