From 1158bb2a058fcdd0c8fc3e60dc77f7a57ddbb271 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:34 -0600 Subject: qapi: Add parameter to visit_end_* Rather than making the dealloc visitor track of stack of pointers remembered during visit_start_* in order to free them during visit_end_*, it's a lot easier to just make all callers pass the same pointer to visit_end_*. The generated code has access to the same pointer, while all other users are doing virtual walks and can pass NULL. The dealloc visitor is then greatly simplified. All three visit_end_*() functions intentionally take a void**, even though the visit_start_*() functions differ between void**, GenericList**, and GenericAlternate**. This is done for several reasons: when doing a virtual walk, passing NULL doesn't care what the type is, but when doing a generated walk, we already have to cast the caller's specific FOO* to call visit_start, while using void** lets us use visit_end without a cast. Also, an upcoming patch will add a clone visitor that wants to use the same implementation for all three visit_end callbacks, which is made easier if all three share the same signature. For visitors with already track per-object state (the QMP visitors via a stack, and the string visitors which do not allow nesting), add an assertion that the caller is indeed passing the same pointer to paired calls. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-4-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- qapi/qapi-dealloc-visitor.c | 47 +++------------------------------------------ 1 file changed, 3 insertions(+), 44 deletions(-) (limited to 'qapi/qapi-dealloc-visitor.c') diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index cd68b55a1a..9391dea203 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -19,53 +19,18 @@ #include "qapi/qmp/types.h" #include "qapi/visitor-impl.h" -typedef struct StackEntry -{ - void *value; - QTAILQ_ENTRY(StackEntry) node; -} StackEntry; - struct QapiDeallocVisitor { Visitor visitor; - QTAILQ_HEAD(, StackEntry) stack; }; -static QapiDeallocVisitor *to_qov(Visitor *v) -{ - return container_of(v, QapiDeallocVisitor, visitor); -} - -static void qapi_dealloc_push(QapiDeallocVisitor *qov, void *value) -{ - StackEntry *e = g_malloc0(sizeof(*e)); - - e->value = value; - - QTAILQ_INSERT_HEAD(&qov->stack, e, node); -} - -static void *qapi_dealloc_pop(QapiDeallocVisitor *qov) -{ - StackEntry *e = QTAILQ_FIRST(&qov->stack); - QObject *value; - QTAILQ_REMOVE(&qov->stack, e, node); - value = e->value; - g_free(e); - return value; -} - static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, size_t unused, Error **errp) { - QapiDeallocVisitor *qov = to_qov(v); - qapi_dealloc_push(qov, obj); } -static void qapi_dealloc_end_struct(Visitor *v) +static void qapi_dealloc_end_struct(Visitor *v, void **obj) { - QapiDeallocVisitor *qov = to_qov(v); - void **obj = qapi_dealloc_pop(qov); if (obj) { g_free(*obj); } @@ -75,14 +40,10 @@ static void qapi_dealloc_start_alternate(Visitor *v, const char *name, GenericAlternate **obj, size_t size, bool promote_int, Error **errp) { - QapiDeallocVisitor *qov = to_qov(v); - qapi_dealloc_push(qov, obj); } -static void qapi_dealloc_end_alternate(Visitor *v) +static void qapi_dealloc_end_alternate(Visitor *v, void **obj) { - QapiDeallocVisitor *qov = to_qov(v); - void **obj = qapi_dealloc_pop(qov); if (obj) { g_free(*obj); } @@ -102,7 +63,7 @@ static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *tail, return next; } -static void qapi_dealloc_end_list(Visitor *v) +static void qapi_dealloc_end_list(Visitor *v, void **obj) { } @@ -178,7 +139,5 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_any = qapi_dealloc_type_anything; v->visitor.type_null = qapi_dealloc_type_null; - QTAILQ_INIT(&v->stack); - return v; } -- cgit v1.2.3-55-g7522 From 2c0ef9f411ae6081efa9eca5b3eab2dbeee45a6c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:35 -0600 Subject: qapi: Add new visit_free() function 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 Message-Id: <1465490926-28625-5-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- docs/qapi-code-gen.txt | 28 ++++++++++------------------ include/qapi/dealloc-visitor.h | 5 +---- include/qapi/visitor-impl.h | 3 +++ include/qapi/visitor.h | 37 ++++++++++++++++++++++++++++++++++--- qapi/opts-visitor.c | 10 ++++++++++ qapi/qapi-dealloc-visitor.c | 14 +++++--------- qapi/qapi-visit-core.c | 7 +++++++ qapi/qmp-input-visitor.c | 8 ++++++++ qapi/qmp-output-visitor.c | 8 ++++++++ qapi/string-input-visitor.c | 8 ++++++++ qapi/string-output-visitor.c | 8 ++++++++ scripts/qapi-commands.py | 16 ++++++---------- scripts/qapi-event.py | 2 +- scripts/qapi-types.py | 6 ++---- tests/test-visitor-serialization.c | 6 +++--- 15 files changed, 114 insertions(+), 52 deletions(-) (limited to 'qapi/qapi-dealloc-visitor.c') diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index 0ae239ad6c..db9d5b6357 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) diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h index 45b06b248c..b3e5c85fd8 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/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index a495bf0937..525b06872b 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/include/qapi/visitor.h b/include/qapi/visitor.h index 25d0cc275a..2ded852307 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/qapi/opts-visitor.c b/qapi/opts-visitor.c index dcfbf92faf..72c95ac7c1 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 9391dea203..e39457bc79 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/qapi-visit-core.c b/qapi/qapi-visit-core.c index dba11c66a7..5f68c251d2 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/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index e16b4b0725..41b7f87ddb 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 dca893584d..5f0035cbf6 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 d7f3c2bd1e..6fb1229586 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -331,6 +331,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); @@ -355,6 +362,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 bdc0fd0be4..ff9ddf068a 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/scripts/qapi-commands.py b/scripts/qapi-commands.py index 971dc4ea8e..77ecd47071 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 084c40a100..909e8d6b8f 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 437cf6c8e3..5ace2cf065 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/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c index db781c0a79..bcbfd2aeb7 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) -- cgit v1.2.3-55-g7522