From patchwork Tue Jan 19 16:10:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 8063961 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 2F5E79F859 for ; Tue, 19 Jan 2016 16:24:39 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4AC1D204A0 for ; Tue, 19 Jan 2016 16:24:38 +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 ED17F20461 for ; Tue, 19 Jan 2016 16:24:34 +0000 (UTC) Received: from localhost ([::1]:37894 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLZ58-0004Co-AI for patchwork-qemu-devel@patchwork.kernel.org; Tue, 19 Jan 2016 11:24:34 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42583) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLYsC-0004yA-AC for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:11:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLYs5-00028X-Th for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:11:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53829) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLYs5-00028A-LB for qemu-devel@nongnu.org; Tue, 19 Jan 2016 11:11:05 -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 5825A357E97; Tue, 19 Jan 2016 16:11:05 +0000 (UTC) Received: from red.redhat.com (ovpn-113-211.phx2.redhat.com [10.3.113.211]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0JGAlYc008625; Tue, 19 Jan 2016 11:11:04 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Tue, 19 Jan 2016 09:10:32 -0700 Message-Id: <1453219845-30939-25-git-send-email-eblake@redhat.com> In-Reply-To: <1453219845-30939-1-git-send-email-eblake@redhat.com> References: <1453219845-30939-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: marcandre.lureau@redhat.com, armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH v9 24/37] 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+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 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), 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), and that it may only be called once per visit. Meanwhile, nothing was using the return value of qmp_output_pop(). Also, adding a parameter will let us diagnose any programming bugs due to mismatched push(struct)/pop(list) or push(list)/pop(struct). To keep the semantics of test_visitor_out_empty, we now have to explicitly request a top-level visit of a NULL object, by implementing the just-added visitor type_null() callback. Signed-off-by: Eric Blake --- 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 + include/qapi/visitor-impl.h | 4 ++-- qapi/qmp-output-visitor.c | 50 +++++++++++++++++++++++---------------- tests/test-qmp-output-visitor.c | 2 ++ 4 files changed, 35 insertions(+), 22 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/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 95408a5..913f1b0 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -75,8 +75,8 @@ struct Visitor * visitors do not currently visit arbitrary types). */ void (*type_any)(Visitor *v, const char *name, QObject **obj, Error **errp); - /* Must be provided to visit explicit null values (right now, only the - * dealloc and qmp-input visitors support this). */ + /* Must be provided to visit explicit null values (the opts and string + * visitors do not currently visit an explicit null). */ void (*type_null)(Visitor *v, const char *name, Error **errp); /* May be NULL; most useful for input visitors. */ diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index df22999..2eb200d 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -1,6 +1,7 @@ /* * Core Definitions for QAPI/QMP Command Registry * + * Copyright (C) 2015-2016 Red Hat, Inc. * Copyright IBM, Corp. 2011 * * Authors: @@ -56,17 +57,15 @@ 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) +/* Remove the most recent QObject with given type from the stack */ +static void qmp_output_pop(QmpOutputVisitor *qov, QType type) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); - QObject *value; assert(e); QTAILQ_REMOVE(&qov->stack, e, node); - value = e->value; + assert(qobject_type(e->value) == type); g_free(e); - return value; } /* Grab the most recent QObject from the stack, if any */ @@ -88,9 +87,8 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, cur = qmp_output_last(qov); 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)) { @@ -99,6 +97,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, qdict_put_obj(qobject_to_qdict(cur), name, value); break; case QTYPE_QLIST: + assert(!name); qlist_append_obj(qobject_to_qlist(cur), value); break; default: @@ -120,7 +119,7 @@ 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); + qmp_output_pop(qov, QTYPE_QDICT); } static void qmp_output_start_list(Visitor *v, const char *name, Error **errp) @@ -151,7 +150,7 @@ 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); + qmp_output_pop(qov, QTYPE_QLIST); } static void qmp_output_type_int64(Visitor *v, const char *name, int64_t *obj, @@ -202,18 +201,22 @@ static void qmp_output_type_any(Visitor *v, const char *name, QObject **obj, qmp_output_add_obj(qov, name, *obj); } +static void qmp_output_type_null(Visitor *v, const char *name, Error **errp) +{ + QmpOutputVisitor *qov = to_qov(v); + qmp_output_add_obj(qov, name, qnull()); +} + /* Finish building, and return the root object. Will not be NULL. */ 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; + QObject *root; + + assert(qov->root); /* A visit must have occurred... */ + assert(!qmp_output_last(qov)); /* ...with each start paired with end. */ + root = qov->root; + qov->root = NULL; + return root; } Visitor *qmp_output_get_visitor(QmpOutputVisitor *v) @@ -221,7 +224,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 +234,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); } @@ -252,6 +261,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void) v->visitor.type_str = qmp_output_type_str; v->visitor.type_number = qmp_output_type_number; v->visitor.type_any = qmp_output_type_any; + v->visitor.type_null = qmp_output_type_null; QTAILQ_INIT(&v->stack); diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 26dc752..74d0ac4 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -260,6 +260,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); } } @@ -459,6 +460,7 @@ static void test_visitor_out_empty(TestOutputVisitorData *data, { QObject *arg; + visit_type_null(data->ov, NULL, &error_abort); arg = qmp_output_get_qobject(data->qov); g_assert(qobject_type(arg) == QTYPE_QNULL); /* Check that qnull reference counting is sane */