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
signature.asc
Description: OpenPGP digital signature
