[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnu
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL |
Date: |
Sat, 12 Sep 2015 07:55:08 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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
- [Qemu-devel] [PATCH v6 01/26] qapi: Rename class QAPISchema to QAPISchemaParser, (continued)
- [Qemu-devel] [PATCH v6 01/26] qapi: Rename class QAPISchema to QAPISchemaParser, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 23/26] qapi-schema: Fix up misleading specification of netdev_add, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 19/26] qapi: Improve built-in type documentation, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 18/26] qapi-commands: De-duplicate output marshaling functions, Markus Armbruster, 2015/09/11
- [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 15/26] qapi-commands: Rearrange code, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 26/26] qapi-introspect: Hide type names, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 24/26] qapi: Pseudo-type '**' is now unused, drop it, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 16/26] qapi: Rename qmp_marshal_input_FOO() to qmp_marshal_FOO(), Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 21/26] qapi: Introduce a first class 'any' type, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 22/26] qom: Don't use 'gen': false for qom-get, qom-set, object-add, Markus Armbruster, 2015/09/11
[Qemu-devel] [PATCH v6 25/26] qapi: New QMP command query-qmp-schema for QMP introspection, Markus Armbruster, 2015/09/11