From patchwork Mon Sep 21 21:57:38 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 520626 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id DF8F714012C for ; Tue, 22 Sep 2015 08:14:44 +1000 (AEST) Received: from localhost ([::1]:34321 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ze9MA-0000rp-Ts for incoming@patchwork.ozlabs.org; Mon, 21 Sep 2015 18:14:42 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ze96O-0007mX-5I for qemu-devel@nongnu.org; Mon, 21 Sep 2015 17:58:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ze96N-0000x6-3j for qemu-devel@nongnu.org; Mon, 21 Sep 2015 17:58:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58729) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ze96M-0000wp-SU for qemu-devel@nongnu.org; Mon, 21 Sep 2015 17:58:23 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 80DECC0B91A3; Mon, 21 Sep 2015 21:58:22 +0000 (UTC) Received: from red.redhat.com (ovpn-113-166.phx2.redhat.com [10.3.113.166]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8LLw4bi027229; Mon, 21 Sep 2015 17:58:21 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 21 Sep 2015 15:57:38 -0600 Message-Id: <1442872682-6523-23-git-send-email-eblake@redhat.com> In-Reply-To: <1442872682-6523-1-git-send-email-eblake@redhat.com> References: <1442872682-6523-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Michael Roth , marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, armbru@redhat.com, ehabkost@redhat.com Subject: [Qemu-devel] [PATCH v5 22/46] qapi: Don't abuse stack to track qmp-output root X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org The previous commit documented an inconsistency in how we are using the stack of qmp-output-visitor. Normally, pushing a single top-level object puts the object on the stack twice: once as the root, and once as the current container being appended to; but popping that struct only pops once. However, qmp_ouput_add() was trying to either set up the added object as the new root (works if you parse two top-level scalars in a row: the second replaces the first as the root) or as a member of the current container (works as long as you have an open container on the stack; but if you have popped the first top-level container, it then resolves to the root and still tries to add into that existing container). Fix the stupidity by not tracking two separate things in the stack. Not done here: maybe qmp_output_get_object() should assert that the stack is empty, rather than letting users look at the current root even while the root is still being visited. --- qapi/qmp-output-visitor.c | 70 +++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 48 deletions(-) diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 850c83d..f891e72 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -29,13 +29,8 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack; struct QmpOutputVisitor { Visitor visitor; - /* FIXME: we are abusing stack to hold two separate pieces of - * information: the current root object, and the stack of objects - * still being built. Worse, our behavior is inconsistent: - * visiting two top-level scalars in a row discards the first in - * favor of the second, but visiting two top-level objects in a - * row tries to append the second object into the first. */ - QStack stack; + QStack stack; /* Stack of containers still growing */ + QObject *root; /* Root of the output visit */ }; #define qmp_output_add(qov, name, value) \ @@ -52,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) { @@ -76,28 +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); - - /* - * FIXME Wrong, because qmp_output_get_qobject() will increment - * the refcnt *again*. We need to think through how visitors - * handle null. - */ - 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. @@ -108,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(); + } } } @@ -242,16 +220,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); }