From patchwork Tue Feb 9 11:38:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 8261771 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 4F9519F1C1 for ; Tue, 9 Feb 2016 13:05:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9E55E20145 for ; Tue, 9 Feb 2016 13:05:24 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AA2CF2012E for ; Tue, 9 Feb 2016 13:05:23 +0000 (UTC) Received: from localhost ([::1]:54723 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT6ra-00079c-5s for patchwork-qemu-devel@patchwork.kernel.org; Tue, 09 Feb 2016 06:53:46 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34361) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT6cc-000853-RO for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:38:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aT6ca-00035l-Fm for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:38:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42556) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT6ca-00035U-9I for qemu-devel@nongnu.org; Tue, 09 Feb 2016 06:38:16 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id E98DC8E387 for ; Tue, 9 Feb 2016 11:38:15 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-34.ams2.redhat.com [10.36.116.34]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u19BcAY6022343 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 9 Feb 2016 06:38:14 -0500 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id E47E13006AF3; Tue, 9 Feb 2016 12:38:04 +0100 (CET) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Tue, 9 Feb 2016 12:38:01 +0100 Message-Id: <1455017883-25867-30-git-send-email-armbru@redhat.com> In-Reply-To: <1455017883-25867-1-git-send-email-armbru@redhat.com> References: <1455017883-25867-1-git-send-email-armbru@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PULL 29/31] qmp: 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+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Eric Blake 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. Drop the now-useless qmp_output_first() and qmp_output_last() while at it. Saved for a later patch: we still are rather sloppy in that qmp_output_get_object() can be called in the middle of a parse, rather than requiring that a visit is complete. Signed-off-by: Eric Blake Message-Id: <1454075341-13658-26-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- qapi/qmp-output-visitor.c | 89 ++++++++++++++--------------------------------- 1 file changed, 26 insertions(+), 63 deletions(-) diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 4500e3b..f47eefa 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -31,16 +31,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 in slot 0, and the stack - * of N objects still being built in slots 1 through N (for N+1 - * slots in use). Worse, our behavior is inconsistent: - * qmp_output_add_obj() 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 (since the first object was placed in the stack - * in both slot 0 and 1, but only popped from slot 1). */ - QStack stack; + QStack stack; /* Stack of containers that haven't yet been finished */ + QObject *root; /* Root of the output visit */ }; #define qmp_output_add(qov, name, value) \ @@ -57,6 +49,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) { @@ -79,60 +72,32 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) return value; } -/* Grab the root QObject, if any */ -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; -} - -/* Peek at the top of the stack of QObjects being built. - * The stack must not be empty. */ -static QObject *qmp_output_last(QmpOutputVisitor *qov) -{ - QStackEntry *e = QTAILQ_FIRST(&qov->stack); - - assert(e && e->value); - 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; + QStackEntry *e = QTAILQ_FIRST(&qov->stack); + QObject *cur = e ? e->value : NULL; - 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 */ - /* FIXME this is abusing the stack; see comment above */ - qobject_decref(qmp_output_pop(qov)); - assert(QTAILQ_EMPTY(&qov->stack)); - qmp_output_push_obj(qov, value); - break; + if (!cur) { + /* FIXME we should require the user to reset the visitor, rather + * than throwing away the previous root */ + 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(); + } } } @@ -233,7 +198,9 @@ static void qmp_output_type_any(Visitor *v, const char *name, QObject **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); + /* FIXME: we should require that a visit occurred, and that it is + * complete (no starts without a matching end) */ + QObject *obj = qov->root; if (obj) { qobject_incref(obj); } else { @@ -251,16 +218,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); }