From patchwork Tue Mar 1 05:14:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 590319 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 E72B9140BA7 for ; Tue, 1 Mar 2016 16:21:34 +1100 (AEDT) Received: from localhost ([::1]:40947 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aackW-00045G-UT for incoming@patchwork.ozlabs.org; Tue, 01 Mar 2016 00:21:32 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59772) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aacdx-0000KO-8u for qemu-devel@nongnu.org; Tue, 01 Mar 2016 00:14:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aacdv-0006R6-Mz for qemu-devel@nongnu.org; Tue, 01 Mar 2016 00:14:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45180) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aacdv-0006Qu-G7 for qemu-devel@nongnu.org; Tue, 01 Mar 2016 00:14:43 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 3A6D463149; Tue, 1 Mar 2016 05:14:43 +0000 (UTC) Received: from red.redhat.com (ovpn-113-165.phx2.redhat.com [10.3.113.165]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u215EZ9W018132; Tue, 1 Mar 2016 00:14:42 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 29 Feb 2016 22:14:26 -0700 Message-Id: <1456809272-14184-13-git-send-email-eblake@redhat.com> In-Reply-To: <1456809272-14184-1-git-send-email-eblake@redhat.com> References: <1456809272-14184-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 01 Mar 2016 05:14:43 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Cc: armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH v12 12/18] qmp: Tighten output visitor rules 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 Add a new qmp_output_visitor_reset(), which must be called before reusing an exising QmpOutputVisitor on a new root object. Tighten assertions to require that qmp_output_get_qobject() can only be called after pairing a visit_end_* for every visit_start_* (rather than allowing it to return a partially built object), and that it must not be called unless at least one visit_type_* or visit_start/ visit_end pair has occurred since creation/reset (the accidental return of NULL fixed by commit ab8bf1d7 would have been much easier to diagnose). Also, check that we are encountering the expected object or list type (both during visit_end*, and also by validating whether 'name' was NULL - although the latter may need to change later if we improve error messages by always passing in a sensible name). This provides protection against mismatched push(struct)/pop(list) or push(list)/pop(struct), similar to the qmp-input protection added in commit bdd8e6b5. Signed-off-by: Eric Blake --- v12: rebase to latest, move type_null() into earlier patches, don't change signature of pop, don't auto-reset after a single get_qobject [no v10, v11] v9: rebase to added patch, squash in more sanity checks, drop Marc-Andre's R-b v8: rename qmp_output_reset to qmp_output_visitor_reset v7: new patch, based on discussion about spapr_drc.c --- include/qapi/qmp-output-visitor.h | 1 + qapi/qmp-output-visitor.c | 39 +++++++++++++++++++++++---------------- tests/test-qmp-output-visitor.c | 6 ++++++ 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/include/qapi/qmp-output-visitor.h b/include/qapi/qmp-output-visitor.h index 2266770..5093f0d 100644 --- a/include/qapi/qmp-output-visitor.h +++ b/include/qapi/qmp-output-visitor.h @@ -21,6 +21,7 @@ typedef struct QmpOutputVisitor QmpOutputVisitor; QmpOutputVisitor *qmp_output_visitor_new(void); void qmp_output_visitor_cleanup(QmpOutputVisitor *v); +void qmp_output_visitor_reset(QmpOutputVisitor *v); QObject *qmp_output_get_qobject(QmpOutputVisitor *v); Visitor *qmp_output_get_visitor(QmpOutputVisitor *v); diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 5681ad3..7c48dfb 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -82,9 +82,8 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, QObject *cur = e ? e->value : NULL; if (!cur) { - /* FIXME we should require the user to reset the visitor, rather - * than throwing away the previous root */ - qobject_decref(qov->root); + /* Don't allow reuse of visitor on more than one root */ + assert(!qov->root); qov->root = value; } else { switch (qobject_type(cur)) { @@ -93,6 +92,9 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, qdict_put_obj(qobject_to_qdict(cur), name, value); break; case QTYPE_QLIST: + /* FIXME: assertion needs adjustment if we fix visit-core + * to pass "name.0" style name during lists. */ + assert(!name); qlist_append_obj(qobject_to_qlist(cur), value); break; default: @@ -114,7 +116,8 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, static void qmp_output_end_struct(Visitor *v, Error **errp) { QmpOutputVisitor *qov = to_qov(v); - qmp_output_pop(qov); + QObject *value = qmp_output_pop(qov); + assert(qobject_type(value) == QTYPE_QDICT); } static void qmp_output_start_list(Visitor *v, const char *name, Error **errp) @@ -145,7 +148,8 @@ static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp, static void qmp_output_end_list(Visitor *v) { QmpOutputVisitor *qov = to_qov(v); - qmp_output_pop(qov); + QObject *value = qmp_output_pop(qov); + assert(qobject_type(value) == QTYPE_QLIST); } static void qmp_output_type_int64(Visitor *v, const char *name, int64_t *obj, @@ -202,18 +206,15 @@ static void qmp_output_type_null(Visitor *v, const char *name, Error **errp) qmp_output_add_obj(qov, name, qnull()); } -/* Finish building, and return the root object. Will not be NULL. */ +/* Finish building, and return the root object. Will not be NULL. + * Caller must use qobject_decref() on the result. */ QObject *qmp_output_get_qobject(QmpOutputVisitor *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 { - obj = qnull(); - } - return obj; + /* A visit must have occurred, with each start paired with end. */ + assert(qov->root && !QTAILQ_FIRST(&qov->stack)); + + qobject_incref(qov->root); + return qov->root; } Visitor *qmp_output_get_visitor(QmpOutputVisitor *v) @@ -221,7 +222,7 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v) return &v->visitor; } -void qmp_output_visitor_cleanup(QmpOutputVisitor *v) +void qmp_output_visitor_reset(QmpOutputVisitor *v) { QStackEntry *e, *tmp; @@ -231,6 +232,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v) } qobject_decref(v->root); + v->root = NULL; +} + +void qmp_output_visitor_cleanup(QmpOutputVisitor *v) +{ + qmp_output_visitor_reset(v); g_free(v); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 4f7692b..e9ebce7 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -138,6 +138,7 @@ static void test_visitor_out_enum(TestOutputVisitorData *data, g_assert_cmpstr(qstring_get_str(qobject_to_qstring(obj)), ==, EnumOne_lookup[i]); qobject_decref(obj); + qmp_output_visitor_reset(data->qov); } } @@ -152,6 +153,7 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data, visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err); g_assert(err); error_free(err); + qmp_output_visitor_reset(data->qov); } } @@ -261,6 +263,7 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data, visit_type_UserDefOne(data->ov, "unused", &pu, &err); g_assert(err); error_free(err); + qmp_output_visitor_reset(data->qov); } } @@ -365,6 +368,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data, qobject_decref(obj); qobject_decref(qobj); + qmp_output_visitor_reset(data->qov); qdict = qdict_new(); qdict_put(qdict, "integer", qint_from_int(-42)); qdict_put(qdict, "boolean", qbool_from_bool(true)); @@ -441,6 +445,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qapi_free_UserDefAlternate(tmp); qobject_decref(arg); + qmp_output_visitor_reset(data->qov); tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QSTRING; tmp->u.s = g_strdup("hello"); @@ -454,6 +459,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, qapi_free_UserDefAlternate(tmp); qobject_decref(arg); + qmp_output_visitor_reset(data->qov); tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QDICT; tmp->u.udfu.integer = 1;