From patchwork Thu Jun 9 16:48:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 9167509 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6D99660467 for ; Thu, 9 Jun 2016 17:06:37 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5817328342 for ; Thu, 9 Jun 2016 17:06:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4CE3628359; Thu, 9 Jun 2016 17:06:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 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.wl.linuxfoundation.org (Postfix) with ESMTPS id 2F70928342 for ; Thu, 9 Jun 2016 17:06:36 +0000 (UTC) Received: from localhost ([::1]:35892 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bB3Pf-0003cR-BB for patchwork-qemu-devel@patchwork.kernel.org; Thu, 09 Jun 2016 13:06:35 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50895) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bB38c-0003da-VG for qemu-devel@nongnu.org; Thu, 09 Jun 2016 12:49:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bB38W-00020X-Su for qemu-devel@nongnu.org; Thu, 09 Jun 2016 12:48:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54545) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bB38W-00020T-KS for qemu-devel@nongnu.org; Thu, 09 Jun 2016 12:48:52 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3048677355; Thu, 9 Jun 2016 16:48:52 +0000 (UTC) Received: from red.redhat.com (ovpn-116-88.phx2.redhat.com [10.3.116.88]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u59GmmCm016618; Thu, 9 Jun 2016 12:48:51 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Thu, 9 Jun 2016 10:48:35 -0600 Message-Id: <1465490926-28625-5-git-send-email-eblake@redhat.com> In-Reply-To: <1465490926-28625-1-git-send-email-eblake@redhat.com> References: <1465490926-28625-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 09 Jun 2016 16:48:52 +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 Subject: [Qemu-devel] [PATCH v5 04/15] qapi: Add new visit_free() function X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: armbru@redhat.com, Michael Roth Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Making each visitor provide its own (awkwardly-named) FOO_cleanup() is unusual, when we can instead have a polymorphic visit_free() interface. Over the next few patches, we can use the polymorphic functions to eliminate the need for a FOO_get_visitor() function for accessing specific visitor functionality, once everything can be accessed directly through the Visitor* interfaces. The dealloc visitor is the first one converted to completely use the new entry point, since qapi_dealloc_visitor_cleanup() was the only reason that qapi_dealloc_get_visitor() existed, and only generated and testsuite code was even using it. With the new visit_free() entry point in place, we no longer need to expose the QapiDeallocVisitor subtype through qapi_dealloc_visitor_new(), and can get by with less generated code, with diffs that look like: | void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj) | { |- QapiDeallocVisitor *qdv; | Visitor *v; | | if (!obj) { | return; | } | |- qdv = qapi_dealloc_visitor_new(); |- v = qapi_dealloc_get_visitor(qdv); |+ v = qapi_dealloc_visitor_new(); | visit_type_ACPIOSTInfo(v, NULL, &obj, NULL); |- qapi_dealloc_visitor_cleanup(qdv); |+ visit_free(v); |} Signed-off-by: Eric Blake --- v5: improve commit message, avoid sensitivity on dealloc struct layout, blank lines after declarations v4: new patch, inspired by review of v3 7/18 --- include/qapi/visitor.h | 37 ++++++++++++++++++++++++++++++++++--- include/qapi/visitor-impl.h | 3 +++ scripts/qapi-commands.py | 16 ++++++---------- scripts/qapi-event.py | 2 +- scripts/qapi-types.py | 6 ++---- include/qapi/dealloc-visitor.h | 5 +---- qapi/qapi-visit-core.c | 7 +++++++ qapi/opts-visitor.c | 10 ++++++++++ qapi/qapi-dealloc-visitor.c | 14 +++++--------- qapi/qmp-input-visitor.c | 8 ++++++++ qapi/qmp-output-visitor.c | 8 ++++++++ qapi/string-input-visitor.c | 8 ++++++++ qapi/string-output-visitor.c | 8 ++++++++ tests/test-visitor-serialization.c | 6 +++--- docs/qapi-code-gen.txt | 28 ++++++++++------------------ 15 files changed, 114 insertions(+), 52 deletions(-) diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 25d0cc2..2ded852 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -37,6 +37,24 @@ * implemented by each visitor, and docs/qapi-code-gen.txt for more * about the QAPI code generator. * + * All of the visitors are created via: + * + * Type *subtype_visitor_new(parameters...); + * + * where Type is either directly 'Visitor *', or is a subtype that can + * be trivially upcast to Visitor * via another function: + * + * Visitor *subtype_get_visitor(SubtypeVisitor *); + * + * A visitor should be used for exactly one top-level visit_type_FOO() + * or virtual walk, then passed to visit_free() to clean up resources. + * It is okay to free the visitor without completing the visit, if + * some other error is detected in the meantime. Output visitors + * provide an additional function, for collecting the final results of + * a successful visit: string_output_get_string() and + * qmp_output_get_qobject(); this collection function should not be + * called if any errors were reported during the visit. + * * All QAPI types have a corresponding function with a signature * roughly compatible with this: * @@ -222,6 +240,19 @@ typedef struct GenericAlternate { char padding[]; } GenericAlternate; +/*** Visitor cleanup ***/ + +/* + * Free @v and any resources it has tied up. + * + * May be called whether or not the visit has been successfully + * completed, but should not be called until a top-level + * visit_type_FOO() or visit_start_ITEM() has been performed on the + * visitor. Safe if @v is NULL. + */ +void visit_free(Visitor *v); + + /*** Visiting structures ***/ /* @@ -272,7 +303,7 @@ void visit_check_struct(Visitor *v, Error **errp); * Must be called after any successful use of visit_start_struct(), * even if intermediate processing was skipped due to errors, to allow * the backend to release any resources. Destroying the visitor early - * behaves as if this was implicitly called. + * with visit_free() behaves as if this was implicitly called. */ void visit_end_struct(Visitor *v, void **obj); @@ -332,7 +363,7 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size); * Must be called after any successful use of visit_start_list(), even * if intermediate processing was skipped due to errors, to allow the * backend to release any resources. Destroying the visitor early - * behaves as if this was implicitly called. + * with visit_free() behaves as if this was implicitly called. */ void visit_end_list(Visitor *v, void **list); @@ -368,7 +399,7 @@ void visit_start_alternate(Visitor *v, const char *name, * Must be called after any successful use of visit_start_alternate(), * even if intermediate processing was skipped due to errors, to allow * the backend to release any resources. Destroying the visitor early - * behaves as if this was implicitly called. + * with visit_free() behaves as if this was implicitly called. * */ void visit_end_alternate(Visitor *v, void **obj); diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index a495bf0..525b068 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -104,6 +104,9 @@ struct Visitor /* Must be set */ VisitorType type; + + /* Must be set */ + void (*free)(Visitor *v); }; #endif diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 971dc4e..77ecd47 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -62,7 +62,6 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, { Error *err = NULL; QmpOutputVisitor *qov = qmp_output_visitor_new(); - QapiDeallocVisitor *qdv; Visitor *v; v = qmp_output_get_visitor(qov); @@ -74,11 +73,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, out: error_propagate(errp, err); - qmp_output_visitor_cleanup(qov); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + visit_free(v); + v = qapi_dealloc_visitor_new(); visit_type_%(c_name)s(v, "unused", &ret_in, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } ''', c_type=ret_type.c_type(), c_name=ret_type.c_name()) @@ -116,7 +114,6 @@ def gen_marshal(name, arg_type, ret_type): if arg_type and arg_type.members: ret += mcgen(''' QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); - QapiDeallocVisitor *qdv; Visitor *v; %(c_name)s arg = {0}; @@ -155,13 +152,12 @@ out: ''') if arg_type and arg_type.members: ret += mcgen(''' - qmp_input_visitor_cleanup(qiv); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + visit_free(v); + v = qapi_dealloc_visitor_new(); visit_start_struct(v, NULL, NULL, 0, NULL); visit_type_%(c_name)s_members(v, &arg, NULL); visit_end_struct(v, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); ''', c_name=arg_type.c_name()) diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 084c40a..909e8d6 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -119,7 +119,7 @@ def gen_event_send(name, arg_type): if arg_type and arg_type.members: ret += mcgen(''' out: - qmp_output_visitor_cleanup(qov); + visit_free(v); ''') ret += mcgen(''' error_propagate(errp, err); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 437cf6c..5ace2cf 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -150,17 +150,15 @@ def gen_type_cleanup(name): void qapi_free_%(c_name)s(%(c_name)s *obj) { - QapiDeallocVisitor *qdv; Visitor *v; if (!obj) { return; } - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + v = qapi_dealloc_visitor_new(); visit_type_%(c_name)s(v, NULL, &obj, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } ''', c_name=c_name(name)) diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h index 45b06b2..b3e5c85 100644 --- a/include/qapi/dealloc-visitor.h +++ b/include/qapi/dealloc-visitor.h @@ -23,9 +23,6 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor; * qapi_free_FOO() functions, and is the only visitor designed to work * correctly in the face of a partially-constructed QAPI tree. */ -QapiDeallocVisitor *qapi_dealloc_visitor_new(void); -void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *d); - -Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v); +Visitor *qapi_dealloc_visitor_new(void); #endif diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index dba11c6..5f68c25 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -20,6 +20,13 @@ #include "qapi/visitor.h" #include "qapi/visitor-impl.h" +void visit_free(Visitor *v) +{ + if (v) { + v->free(v); + } +} + void visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index dcfbf92..72c95ac 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -513,6 +513,15 @@ opts_optional(Visitor *v, const char *name, bool *present) } +static void +opts_free(Visitor *v) +{ + OptsVisitor *ov = to_ov(v); + + opts_visitor_cleanup(ov); +} + + OptsVisitor * opts_visitor_new(const QemuOpts *opts) { @@ -540,6 +549,7 @@ opts_visitor_new(const QemuOpts *opts) * skip some mandatory methods... */ ov->visitor.optional = &opts_optional; + ov->visitor.free = opts_free; ov->opts_root = opts; diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 9391dea..e39457b 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -107,17 +107,12 @@ static void qapi_dealloc_type_null(Visitor *v, const char *name, Error **errp) { } -Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) +static void qapi_dealloc_free(Visitor *v) { - return &v->visitor; + g_free(container_of(v, QapiDeallocVisitor, visitor)); } -void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *v) -{ - g_free(v); -} - -QapiDeallocVisitor *qapi_dealloc_visitor_new(void) +Visitor *qapi_dealloc_visitor_new(void) { QapiDeallocVisitor *v; @@ -138,6 +133,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_number = qapi_dealloc_type_number; v->visitor.type_any = qapi_dealloc_type_anything; v->visitor.type_null = qapi_dealloc_type_null; + v->visitor.free = qapi_dealloc_free; - return v; + return &v->visitor; } diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index e16b4b0..41b7f87 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -378,6 +378,13 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v) return &v->visitor; } +static void qmp_input_free(Visitor *v) +{ + QmpInputVisitor *qiv = to_qiv(v); + + qmp_input_visitor_cleanup(qiv); +} + void qmp_input_visitor_cleanup(QmpInputVisitor *v) { qobject_decref(v->root); @@ -406,6 +413,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict) v->visitor.type_any = qmp_input_type_any; v->visitor.type_null = qmp_input_type_null; v->visitor.optional = qmp_input_optional; + v->visitor.free = qmp_input_free; v->strict = strict; v->root = obj; diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index dca8935..5f0035c 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -214,6 +214,13 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v) return &v->visitor; } +static void qmp_output_free(Visitor *v) +{ + QmpOutputVisitor *qov = to_qov(v); + + qmp_output_visitor_cleanup(qov); +} + void qmp_output_visitor_cleanup(QmpOutputVisitor *v) { QStackEntry *e, *tmp; @@ -246,6 +253,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void) v->visitor.type_number = qmp_output_type_number; v->visitor.type_any = qmp_output_type_any; v->visitor.type_null = qmp_output_type_null; + v->visitor.free = qmp_output_free; QTAILQ_INIT(&v->stack); diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 68afc4e..abf396b 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -335,6 +335,13 @@ Visitor *string_input_get_visitor(StringInputVisitor *v) return &v->visitor; } +static void string_input_free(Visitor *v) +{ + StringInputVisitor *siv = to_siv(v); + + string_input_visitor_cleanup(siv); +} + void string_input_visitor_cleanup(StringInputVisitor *v) { g_list_foreach(v->ranges, free_range, NULL); @@ -359,6 +366,7 @@ StringInputVisitor *string_input_visitor_new(const char *str) v->visitor.next_list = next_list; v->visitor.end_list = end_list; v->visitor.optional = parse_optional; + v->visitor.free = string_input_free; v->string = str; return v; diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 0c46dda..6933c8e 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -322,6 +322,13 @@ static void free_range(void *range, void *dummy) g_free(range); } +static void string_output_free(Visitor *v) +{ + StringOutputVisitor *sov = to_sov(v); + + string_output_visitor_cleanup(sov); +} + void string_output_visitor_cleanup(StringOutputVisitor *sov) { if (sov->string) { @@ -351,6 +358,7 @@ StringOutputVisitor *string_output_visitor_new(bool human) v->visitor.start_list = start_list; v->visitor.next_list = next_list; v->visitor.end_list = end_list; + v->visitor.free = string_output_free; return v; } diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index db781c0..bcbfd2a 100644 --- a/tests/test-visitor-serialization.c +++ b/tests/test-visitor-serialization.c @@ -89,11 +89,11 @@ typedef void (*VisitorFunc)(Visitor *v, void **native, Error **errp); static void dealloc_helper(void *native_in, VisitorFunc visit, Error **errp) { - QapiDeallocVisitor *qdv = qapi_dealloc_visitor_new(); + Visitor *v = qapi_dealloc_visitor_new(); - visit(qapi_dealloc_get_visitor(qdv), &native_in, errp); + visit(v, &native_in, errp); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } static void visit_primitive_type(Visitor *v, void **native, Error **errp) diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 0ae239a..db9d5b6 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -802,32 +802,28 @@ Example: void qapi_free_UserDefOne(UserDefOne *obj) { - QapiDeallocVisitor *qdv; Visitor *v; if (!obj) { return; } - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + v = qapi_dealloc_visitor_new(); visit_type_UserDefOne(v, NULL, &obj, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } void qapi_free_UserDefOneList(UserDefOneList *obj) { - QapiDeallocVisitor *qdv; Visitor *v; if (!obj) { return; } - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + v = qapi_dealloc_visitor_new(); visit_type_UserDefOneList(v, NULL, &obj, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } === scripts/qapi-visit.py === @@ -985,7 +981,6 @@ Example: { Error *err = NULL; QmpOutputVisitor *qov = qmp_output_visitor_new(); - QapiDeallocVisitor *qdv; Visitor *v; v = qmp_output_get_visitor(qov); @@ -997,11 +992,10 @@ Example: out: error_propagate(errp, err); - qmp_output_visitor_cleanup(qov); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + visit_free(v); + v = qapi_dealloc_visitor_new(); visit_type_UserDefOne(v, "unused", &ret_in, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } static void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp) @@ -1009,7 +1003,6 @@ Example: Error *err = NULL; UserDefOne *retval; QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); - QapiDeallocVisitor *qdv; Visitor *v; UserDefOneList *arg1 = NULL; @@ -1036,13 +1029,12 @@ Example: out: error_propagate(errp, err); - qmp_input_visitor_cleanup(qiv); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); + visit_free(v); + v = qapi_dealloc_visitor_new(); visit_start_struct(v, NULL, NULL, 0, NULL); visit_type_UserDefOneList(v, "arg1", &arg1, NULL); visit_end_struct(v, NULL); - qapi_dealloc_visitor_cleanup(qdv); + visit_free(v); } static void qmp_init_marshal(void)