From c7eb39cbd4304ca55c760c0cb33ce235c457546c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:32 -0600 Subject: qapi: Improve use of qmp/types.h 'qjson.h' is not a QObject subtype; include this file directly in .c files that are using it, rather than abusing qmp/types.h for that purpose. Meanwhile, for files that include a list of individual QObject subtypes, it's easier to just use qmp/types.h for that purpose. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-2-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/qmp/types.h | 1 - 1 file changed, 1 deletion(-) (limited to 'include/qapi') diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h index 7782ec5..f21ecf4 100644 --- a/include/qapi/qmp/types.h +++ b/include/qapi/qmp/types.h @@ -20,6 +20,5 @@ #include "qapi/qmp/qstring.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qlist.h" -#include "qapi/qmp/qjson.h" #endif /* QEMU_OBJECTS_H */ -- cgit v1.1 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 --- include/qapi/visitor-impl.h | 6 +++--- include/qapi/visitor.h | 32 +++++++++++++++++++------------- 2 files changed, 22 insertions(+), 16 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 145afd0..a495bf0 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -47,7 +47,7 @@ struct Visitor void (*check_struct)(Visitor *v, Error **errp); /* Must be set to visit structs */ - void (*end_struct)(Visitor *v); + void (*end_struct)(Visitor *v, void **obj); /* Must be set; implementations may require @list to be non-null, * but must document it. */ @@ -58,7 +58,7 @@ struct Visitor GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size); /* Must be set */ - void (*end_list)(Visitor *v); + void (*end_list)(Visitor *v, void **list); /* Must be set by input and dealloc visitors to visit alternates; * optional for output visitors. */ @@ -67,7 +67,7 @@ struct Visitor bool promote_int, Error **errp); /* Optional, needed for dealloc visitor */ - void (*end_alternate)(Visitor *v); + void (*end_alternate)(Visitor *v, void **obj); /* Must be set */ void (*type_int64)(Visitor *v, const char *name, int64_t *obj, diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 4d12167..25d0cc2 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -193,12 +193,12 @@ * goto outlist; * } * outlist: - * visit_end_list(v); + * visit_end_list(v, NULL); * if (!err) { * visit_check_struct(v, &err); * } * outobj: - * visit_end_struct(v); + * visit_end_struct(v, NULL); * out: * error_propagate(errp, err); * ...clean up v... @@ -242,8 +242,8 @@ typedef struct GenericAlternate { * After visit_start_struct() succeeds, the caller may visit its * members one after the other, passing the member's name and address * within the struct. Finally, visit_end_struct() needs to be called - * to clean up, even if intermediate visits fail. See the examples - * above. + * with the same @obj to clean up, even if intermediate visits fail. + * See the examples above. * * FIXME Should this be named visit_start_object, since it is also * used for QAPI unions, and maps to JSON objects? @@ -267,12 +267,14 @@ void visit_check_struct(Visitor *v, Error **errp); /* * Complete an object visit started earlier. * + * @obj must match what was passed to the paired visit_start_struct(). + * * 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. */ -void visit_end_struct(Visitor *v); +void visit_end_struct(Visitor *v, void **obj); /*** Visiting lists ***/ @@ -299,8 +301,9 @@ void visit_end_struct(Visitor *v); * visit (where @obj is NULL) uses other means. For each list * element, call the appropriate visit_type_FOO() with name set to * NULL and obj set to the address of the value member of the list - * element. Finally, visit_end_list() needs to be called to clean up, - * even if intermediate visits fail. See the examples above. + * element. Finally, visit_end_list() needs to be called with the + * same @list to clean up, even if intermediate visits fail. See the + * examples above. */ void visit_start_list(Visitor *v, const char *name, GenericList **list, size_t size, Error **errp); @@ -324,12 +327,14 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size); /* * Complete a list visit started earlier. * + * @list must match what was passed to the paired visit_start_list(). + * * 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. */ -void visit_end_list(Visitor *v); +void visit_end_list(Visitor *v, void **list); /*** Visiting alternates ***/ @@ -347,8 +352,9 @@ void visit_end_list(Visitor *v); * * If @promote_int, treat integers as QTYPE_FLOAT. * - * If successful, this must be paired with visit_end_alternate() to - * clean up, even if visiting the contents of the alternate fails. + * If successful, this must be paired with visit_end_alternate() with + * the same @obj to clean up, even if visiting the contents of the + * alternate fails. */ void visit_start_alternate(Visitor *v, const char *name, GenericAlternate **obj, size_t size, @@ -357,15 +363,15 @@ void visit_start_alternate(Visitor *v, const char *name, /* * Finish visiting an alternate type. * + * @obj must match what was passed to the paired visit_start_alternate(). + * * 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. * - * TODO: Should all the visit_end_* interfaces take obj parameter, so - * that dealloc visitor need not track what was passed in visit_start? */ -void visit_end_alternate(Visitor *v); +void visit_end_alternate(Visitor *v, void **obj); /*** Other helpers ***/ -- cgit v1.1 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 --- include/qapi/dealloc-visitor.h | 5 +---- include/qapi/visitor-impl.h | 3 +++ include/qapi/visitor.h | 37 ++++++++++++++++++++++++++++++++++--- 3 files changed, 38 insertions(+), 7 deletions(-) (limited to 'include/qapi') 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/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/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); -- cgit v1.1 From 09204eac9bb513e56992c00c75f32f9d4766256b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:36 -0600 Subject: opts-visitor: Favor new visit_free() function Now that we have a polymorphic visit_free(), we no longer need opts_visitor_cleanup(); which in turn means we no longer need to return a subtype from opts_visitor_new() nor a public upcast function. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-6-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/opts-visitor.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h index ae1bf7c..6462c96 100644 --- a/include/qapi/opts-visitor.h +++ b/include/qapi/opts-visitor.h @@ -35,8 +35,6 @@ typedef struct OptsVisitor OptsVisitor; * QTypes. It also requires a non-null list argument to * visit_start_list(). */ -OptsVisitor *opts_visitor_new(const QemuOpts *opts); -void opts_visitor_cleanup(OptsVisitor *nv); -Visitor *opts_get_visitor(OptsVisitor *nv); +Visitor *opts_visitor_new(const QemuOpts *opts); #endif -- cgit v1.1 From 7a0525c7be6b38d32d586e3fd12e7377ded21faa Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:37 -0600 Subject: string-input-visitor: Favor new visit_free() function Now that we have a polymorphic visit_free(), we no longer need string_input_visitor_cleanup(); which in turn means we no longer need to return a subtype from string_input_visitor_new() nor a public upcast function. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-7-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/string-input-visitor.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h index 7b76c2b..3355134 100644 --- a/include/qapi/string-input-visitor.h +++ b/include/qapi/string-input-visitor.h @@ -22,9 +22,6 @@ typedef struct StringInputVisitor StringInputVisitor; * QAPI structs, alternates, null, or arbitrary QTypes. It also * requires a non-null list argument to visit_start_list(). */ -StringInputVisitor *string_input_visitor_new(const char *str); -void string_input_visitor_cleanup(StringInputVisitor *v); - -Visitor *string_input_get_visitor(StringInputVisitor *v); +Visitor *string_input_visitor_new(const char *str); #endif -- cgit v1.1 From b70ce1018a251c0c33498d9c927a07cade655a5e Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:38 -0600 Subject: qmp-input-visitor: Favor new visit_free() function Now that we have a polymorphic visit_free(), we no longer need qmp_input_visitor_cleanup(); which in turn means we no longer need to return a subtype from qmp_input_visitor_new() nor a public upcast function. Generated code changes to qmp-marshal.c look like: |@@ -52,11 +52,10 @@ void qmp_marshal_add_fd(QDict *args, QOb | { | Error *err = NULL; | AddfdInfo *retval; |- QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true); | Visitor *v; | q_obj_add_fd_arg arg = {0}; | |- v = qmp_input_get_visitor(qiv); |+ v = qmp_input_visitor_new(QOBJECT(args), true); | visit_start_struct(v, NULL, NULL, 0, &err); | if (err) { | goto out; Signed-off-by: Eric Blake Message-Id: <1465490926-28625-8-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/qmp-input-visitor.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/qmp-input-visitor.h b/include/qapi/qmp-input-visitor.h index b0624d8..f3ff5f3 100644 --- a/include/qapi/qmp-input-visitor.h +++ b/include/qapi/qmp-input-visitor.h @@ -25,10 +25,6 @@ typedef struct QmpInputVisitor QmpInputVisitor; * Set @strict to reject a parse that doesn't consume all keys of a * dictionary; otherwise excess input is ignored. */ -QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool strict); - -void qmp_input_visitor_cleanup(QmpInputVisitor *v); - -Visitor *qmp_input_get_visitor(QmpInputVisitor *v); +Visitor *qmp_input_visitor_new(QObject *obj, bool strict); #endif -- cgit v1.1 From e7ca56562990991bc614a43b9351ee0737f3045d Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:39 -0600 Subject: string-output-visitor: Favor new visit_free() function Now that we have a polymorphic visit_free(), we no longer need string_output_visitor_cleanup(); however, we still need to expose the subtype for string_output_get_string(). Signed-off-by: Eric Blake Message-Id: <1465490926-28625-9-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/string-output-visitor.h | 1 - 1 file changed, 1 deletion(-) (limited to 'include/qapi') diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h index e10522a..03e377e 100644 --- a/include/qapi/string-output-visitor.h +++ b/include/qapi/string-output-visitor.h @@ -23,7 +23,6 @@ typedef struct StringOutputVisitor StringOutputVisitor; * requires a non-null list argument to visit_start_list(). */ StringOutputVisitor *string_output_visitor_new(bool human); -void string_output_visitor_cleanup(StringOutputVisitor *v); char *string_output_get_string(StringOutputVisitor *v); Visitor *string_output_get_visitor(StringOutputVisitor *v); -- cgit v1.1 From 1830f22a6777cedaccd67a08f675d30f7a85ebfd Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:40 -0600 Subject: qmp-output-visitor: Favor new visit_free() function Now that we have a polymorphic visit_free(), we no longer need qmp_output_visitor_cleanup(); however, we still need to expose the subtype for qmp_output_get_qobject(). Signed-off-by: Eric Blake Message-Id: <1465490926-28625-10-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/qmp-output-visitor.h | 1 - 1 file changed, 1 deletion(-) (limited to 'include/qapi') diff --git a/include/qapi/qmp-output-visitor.h b/include/qapi/qmp-output-visitor.h index 2266770..29c9a2e 100644 --- a/include/qapi/qmp-output-visitor.h +++ b/include/qapi/qmp-output-visitor.h @@ -20,7 +20,6 @@ typedef struct QmpOutputVisitor QmpOutputVisitor; QmpOutputVisitor *qmp_output_visitor_new(void); -void qmp_output_visitor_cleanup(QmpOutputVisitor *v); QObject *qmp_output_get_qobject(QmpOutputVisitor *v); Visitor *qmp_output_get_visitor(QmpOutputVisitor *v); -- cgit v1.1 From 3b098d56979d2f7fd707c5be85555d114353a28d Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:43 -0600 Subject: qapi: Add new visit_complete() function Making each output visitor provide its own output collection function was the only remaining reason for exposing visitor sub-types to the rest of the code base. Add a polymorphic visit_complete() function which is a no-op for input visitors, and which populates an opaque pointer for output visitors. For maximum type-safety, also add a parameter to the output visitor constructors with a type-correct version of the output pointer, and assert that the two uses match. This approach was considered superior to either passing the output parameter only during construction (action at a distance during visit_free() feels awkward) or only during visit_complete() (defeating type safety makes it easier to use incorrectly). Most callers were function-local, and therefore a mechanical conversion; the testsuite was a bit trickier, but the previous cleanup patch minimized the churn here. The visit_complete() function may be called at most once; doing so lets us use transfer semantics rather than duplication or ref-count semantics to get the just-built output back to the caller, even though it means our behavior is not idempotent. Generated code is simplified as follows for events: |@@ -26,7 +26,7 @@ void qapi_event_send_acpi_device_ost(ACP | QDict *qmp; | Error *err = NULL; | QMPEventFuncEmit emit; |- QmpOutputVisitor *qov; |+ QObject *obj; | Visitor *v; | q_obj_ACPI_DEVICE_OST_arg param = { | info |@@ -39,8 +39,7 @@ void qapi_event_send_acpi_device_ost(ACP | | qmp = qmp_event_build_dict("ACPI_DEVICE_OST"); | |- qov = qmp_output_visitor_new(); |- v = qmp_output_get_visitor(qov); |+ v = qmp_output_visitor_new(&obj); | | visit_start_struct(v, "ACPI_DEVICE_OST", NULL, 0, &err); | if (err) { |@@ -55,7 +54,8 @@ void qapi_event_send_acpi_device_ost(ACP | goto out; | } | |- qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); |+ visit_complete(v, &obj); |+ qdict_put_obj(qmp, "data", obj); | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err); and for commands: | { | Error *err = NULL; |- QmpOutputVisitor *qov = qmp_output_visitor_new(); | Visitor *v; | |- v = qmp_output_get_visitor(qov); |+ v = qmp_output_visitor_new(ret_out); | visit_type_AddfdInfo(v, "unused", &ret_in, &err); |- if (err) { |- goto out; |+ if (!err) { |+ visit_complete(v, ret_out); | } |- *ret_out = qmp_output_get_qobject(qov); |- |-out: | error_propagate(errp, err); Signed-off-by: Eric Blake Message-Id: <1465490926-28625-13-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/qmp-output-visitor.h | 11 +++++--- include/qapi/string-output-visitor.h | 13 +++++++--- include/qapi/visitor-impl.h | 3 +++ include/qapi/visitor.h | 50 +++++++++++++++++++++--------------- 4 files changed, 49 insertions(+), 28 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/qmp-output-visitor.h b/include/qapi/qmp-output-visitor.h index 29c9a2e..040fdda 100644 --- a/include/qapi/qmp-output-visitor.h +++ b/include/qapi/qmp-output-visitor.h @@ -19,9 +19,12 @@ typedef struct QmpOutputVisitor QmpOutputVisitor; -QmpOutputVisitor *qmp_output_visitor_new(void); - -QObject *qmp_output_get_qobject(QmpOutputVisitor *v); -Visitor *qmp_output_get_visitor(QmpOutputVisitor *v); +/* + * Create a new QMP output visitor. + * + * If everything else succeeds, pass @result to visit_complete() to + * collect the result of the visit. + */ +Visitor *qmp_output_visitor_new(QObject **result); #endif diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h index 03e377e..268dfe9 100644 --- a/include/qapi/string-output-visitor.h +++ b/include/qapi/string-output-visitor.h @@ -18,13 +18,18 @@ typedef struct StringOutputVisitor StringOutputVisitor; /* + * Create a new string output visitor. + * + * Using @human creates output that is a bit easier for humans to read + * (for example, showing integer values in both decimal and hex). + * + * If everything else succeeds, pass @result to visit_complete() to + * collect the result of the visit. + * * The string output visitor does not implement support for visiting * QAPI structs, alternates, null, or arbitrary QTypes. It also * requires a non-null list argument to visit_start_list(). */ -StringOutputVisitor *string_output_visitor_new(bool human); - -char *string_output_get_string(StringOutputVisitor *v); -Visitor *string_output_get_visitor(StringOutputVisitor *v); +Visitor *string_output_visitor_new(bool human, char **result); #endif diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 525b068..16e0b86 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -105,6 +105,9 @@ struct Visitor /* Must be set */ VisitorType type; + /* Must be set for output visitors, optional otherwise. */ + void (*complete)(Visitor *v, void *opaque); + /* Must be set */ void (*free)(Visitor *v); }; diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 2ded852..00a60ea 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -39,21 +39,15 @@ * * 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 *); + * Visitor *subtype_visitor_new(parameters...); * * 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. + * or virtual walk; if that is successful, the caller can optionally + * call visit_complete() (for now, useful only for output visits, but + * safe to call on all visits). Then, regardless of success or + * failure, the user should call 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. + * some other error is detected in the meantime. * * All QAPI types have a corresponding function with a signature * roughly compatible with this: @@ -123,14 +117,14 @@ * Error *err = NULL; * Visitor *v; * - * v = ...obtain input visitor... + * v = FOO_visitor_new(...); * visit_type_Foo(v, NULL, &f, &err); * if (err) { * ...handle error... * } else { * ...use f... * } - * ...clean up v... + * visit_free(v); * qapi_free_Foo(f); * * @@ -140,7 +134,7 @@ * Error *err = NULL; * Visitor *v; * - * v = ...obtain input visitor... + * v = FOO_visitor_new(...); * visit_type_FooList(v, NULL, &l, &err); * if (err) { * ...handle error... @@ -149,7 +143,7 @@ * ...use l->value... * } * } - * ...clean up v... + * visit_free(v); * qapi_free_FooList(l); * * @@ -159,13 +153,17 @@ * Foo *f = ...obtain populated object... * Error *err = NULL; * Visitor *v; + * Type *result; * - * v = ...obtain output visitor... + * v = FOO_visitor_new(..., &result); * visit_type_Foo(v, NULL, &f, &err); * if (err) { * ...handle error... + * } else { + * visit_complete(v, &result); + * ...use result... * } - * ...clean up v... + * visit_free(v); * * * When visiting a real QAPI struct, this file provides several @@ -191,7 +189,7 @@ * Error *err = NULL; * int value; * - * v = ...obtain visitor... + * v = FOO_visitor_new(...); * visit_start_struct(v, NULL, NULL, 0, &err); * if (err) { * goto out; @@ -219,7 +217,7 @@ * visit_end_struct(v, NULL); * out: * error_propagate(errp, err); - * ...clean up v... + * visit_free(v); * */ @@ -243,6 +241,18 @@ typedef struct GenericAlternate { /*** Visitor cleanup ***/ /* + * Complete the visit, collecting any output. + * + * May only be called only once after a successful top-level + * visit_type_FOO() or visit_end_ITEM(), and marks the end of the + * visit. The @opaque pointer should match the output parameter + * passed to the subtype_visitor_new() used to create an output + * visitor, or NULL for any other visitor. Needed for output + * visitors, but may also be called with other visitors. + */ +void visit_complete(Visitor *v, void *opaque); + +/* * Free @v and any resources it has tied up. * * May be called whether or not the visit has been successfully -- cgit v1.1 From a15fcc3cf69ee3d408f60d6cc316488d2b0249b4 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:44 -0600 Subject: qapi: Add new clone visitor We have a couple places in the code base that want to deep-clone one QAPI object into another, and they were resorting to serializing the struct out to QObject then reparsing it. A much more efficient version can be done by adding a new clone visitor. Since cloning is still relatively uncommon, expose the use of the new visitor via a QAPI_CLONE() macro that takes care of type-punning the underlying function pointer, rather than generating lots of unused functions for types that won't be cloned. And yes, we're relying on the compiler treating all pointers equally, even though a strict C program cannot portably do so - but we're not the first one in the qemu code base to expect it to work (hello, glib!). The choice of adding a fourth visitor type deserves some explanation. On the surface, the clone visitor is mostly an input visitor (it takes arbitrary input - in this case, another QAPI object - and creates a new QAPI object during the course of the visit). But ever since commit da72ab0 consolidated enum visits based on the visitor type, using VISITOR_INPUT would cause us to run visit_type_str(), even though for cloning there is nothing to do (we just copy the enum value across, without regards to its mapping to strings). Also, since our input happens to be a QAPI object, we can also satisfy the internal checks for VISITOR_OUTPUT. So in the end, I settled with a new VISITOR_CLONE, and chose its value such that many internal checks can use 'v->type & mask', sticking to 'v->type == value' where the difference matters. Note that we can only clone objects (including alternates) and lists, not built-ins or enums. The visitor core hides integer width from the actual visitor (since commit 04e070d), and as long as that's the case, we can't clone top-level integers. Then again, those can always be cloned by direct copy, since they are not objects with deep pointers, so it's no real loss. And restricting cloning to just objects and lists is cleaner than restricting it to non-integers. As such, I documented that the clone visitor is for direct use only by code internal to QAPI, and should not be used on incomplete objects (other than a hack to work around the fact that we allow NULL in place of "" in visit_type_str() in other output visitors). Note that as written, the clone visitor will never fail on a complete object. Scalars (including enums) not at the root of the clone copy just fine with no additional effort while visiting the scalar, by virtue of a g_memdup() each time we push another struct onto the stack. Cloning a string requires deduplication of a pointer, which means it can also provide the guarantee of an input visitor of never producing NULL even when still accepting NULL in place of "" the way the QMP output visitor does. Cloning an 'any' type could be possible by incrementing the QObject refcnt, but it's not obvious whether that is better than implementing a QObject deep clone. So for now, we document it as unsupported, and intentionally omit the .type_any() callback to let a developer know their usage needs implementation. Add testsuite coverage for several different clone situations, to ensure that the code is working. I also tested that valgrind was happy with the test. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-14-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/clone-visitor.h | 37 +++++++++++++++++++++++++ include/qapi/visitor-impl.h | 14 ++++++---- include/qapi/visitor.h | 66 +++++++++++++++++++++++++------------------- 3 files changed, 84 insertions(+), 33 deletions(-) create mode 100644 include/qapi/clone-visitor.h (limited to 'include/qapi') diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h new file mode 100644 index 0000000..16ceff5 --- /dev/null +++ b/include/qapi/clone-visitor.h @@ -0,0 +1,37 @@ +/* + * Clone Visitor + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef QAPI_CLONE_VISITOR_H +#define QAPI_CLONE_VISITOR_H + +#include "qapi/visitor.h" + +/* + * The clone visitor is for direct use only by the QAPI_CLONE() macro; + * it requires that the root visit occur on an object, list, or + * alternate, and is not usable directly on built-in QAPI types. + */ +typedef struct QapiCloneVisitor QapiCloneVisitor; + +void *qapi_clone(const void *src, void (*visit_type)(Visitor *, const char *, + void **, Error **)); + +/* + * Deep-clone QAPI object @src of the given @type, and return the result. + * + * Not usable on QAPI scalars (integers, strings, enums), nor on a + * QAPI object that references the 'any' type. Safe when @src is NULL. + */ +#define QAPI_CLONE(type, src) \ + ((type *)qapi_clone(src, \ + (void (*)(Visitor *, const char *, void**, \ + Error **))visit_type_ ## type)) + +#endif diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 16e0b86..8bd47ee 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -27,14 +27,18 @@ */ /* - * There are three classes of visitors; setting the class determines + * There are four classes of visitors; setting the class determines * how QAPI enums are visited, as well as what additional restrictions - * can be asserted. + * can be asserted. The values are intentionally chosen so as to + * permit some assertions based on whether a given bit is set (that + * is, some assertions apply to input and clone visitors, some + * assertions apply to output and clone visitors). */ typedef enum VisitorType { - VISITOR_INPUT, - VISITOR_OUTPUT, - VISITOR_DEALLOC, + VISITOR_INPUT = 1, + VISITOR_OUTPUT = 2, + VISITOR_CLONE = 3, + VISITOR_DEALLOC = 4, } VisitorType; struct Visitor diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 00a60ea..fb8f4eb 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -24,15 +24,16 @@ * for doing work at each node of a QAPI graph; it can also be used * for a virtual walk, where there is no actual QAPI C struct. * - * There are three kinds of visitor classes: input visitors (QMP, + * There are four kinds of visitor classes: input visitors (QMP, * string, and QemuOpts) parse an external representation and build * the corresponding QAPI graph, output visitors (QMP and string) take - * a completed QAPI graph and generate an external representation, and - * the dealloc visitor can take a QAPI graph (possibly partially - * constructed) and recursively free its resources. While the dealloc - * and QMP input/output visitors are general, the string and QemuOpts - * visitors have some implementation limitations; see the - * documentation for each visitor for more details on what it + * a completed QAPI graph and generate an external representation, the + * dealloc visitor can take a QAPI graph (possibly partially + * constructed) and recursively free its resources, and the clone + * visitor performs a deep clone of one QAPI object to another. While + * the dealloc and QMP input/output visitors are general, the string, + * QemuOpts, and clone visitors have some implementation limitations; + * see the documentation for each visitor for more details on what it * supports. Also, see visitor-impl.h for the callback contracts * implemented by each visitor, and docs/qapi-code-gen.txt for more * about the QAPI code generator. @@ -80,9 +81,9 @@ * * If an error is detected during visit_type_FOO() with an input * visitor, then *@obj will be NULL for pointer types, and left - * unchanged for scalar types. Using an output visitor with an - * incomplete object has undefined behavior (other than a special case - * for visit_type_str() treating NULL like ""), while the dealloc + * unchanged for scalar types. Using an output or clone visitor with + * an incomplete object has undefined behavior (other than a special + * case for visit_type_str() treating NULL like ""), while the dealloc * visitor safely handles incomplete objects. Since input visitors * never produce an incomplete object, such an object is possible only * by manual construction. @@ -102,11 +103,19 @@ * * void qapi_free_FOO(FOO *obj); * - * which behaves like free() in that @obj may be NULL. Because of - * these functions, the dealloc visitor is seldom used directly - * outside of generated code. QAPI types can also inherit from a base - * class; when this happens, a function is generated for easily going - * from the derived type to the base type: + * where behaves like free() in that @obj may be NULL. Such objects + * may also be used with the following macro, provided alongside the + * clone visitor: + * + * Type *QAPI_CLONE(Type, src); + * + * in order to perform a deep clone of @src. Because of the generated + * qapi_free functions and the QAPI_CLONE() macro, the clone and + * dealloc visitor should not be used directly outside of QAPI code. + * + * QAPI types can also inherit from a base class; when this happens, a + * function is generated for easily going from the derived type to the + * base type: * * BASE *qapi_CHILD_base(CHILD *obj); * @@ -272,9 +281,9 @@ void visit_free(Visitor *v); * container; see the general description of @name above. * * @obj must be non-NULL for a real walk, in which case @size - * determines how much memory an input visitor will allocate into - * *@obj. @obj may also be NULL for a virtual walk, in which case - * @size is ignored. + * determines how much memory an input or clone visitor will allocate + * into *@obj. @obj may also be NULL for a virtual walk, in which + * case @size is ignored. * * @errp obeys typical error usage, and reports failures such as a * member @name is not present, or present but not an object. On @@ -327,9 +336,9 @@ void visit_end_struct(Visitor *v, void **obj); * container; see the general description of @name above. * * @list must be non-NULL for a real walk, in which case @size - * determines how much memory an input visitor will allocate into - * *@list (at least sizeof(GenericList)). Some visitors also allow - * @list to be NULL for a virtual walk, in which case @size is + * determines how much memory an input or clone visitor will allocate + * into *@list (at least sizeof(GenericList)). Some visitors also + * allow @list to be NULL for a virtual walk, in which case @size is * ignored. * * @errp obeys typical error usage, and reports failures such as a @@ -386,10 +395,10 @@ void visit_end_list(Visitor *v, void **list); * @name expresses the relationship of this alternate to its parent * container; see the general description of @name above. * - * @obj must not be NULL. Input visitors use @size to determine how - * much memory to allocate into *@obj, then determine the qtype of the - * next thing to be visited, stored in (*@obj)->type. Other visitors - * will leave @obj unchanged. + * @obj must not be NULL. Input and clone visitors use @size to + * determine how much memory to allocate into *@obj, then determine + * the qtype of the next thing to be visited, stored in (*@obj)->type. + * Other visitors will leave @obj unchanged. * * If @promote_int, treat integers as QTYPE_FLOAT. * @@ -554,9 +563,10 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); * @name expresses the relationship of this string to its parent * container; see the general description of @name above. * - * @obj must be non-NULL. Input visitors set *@obj to the value - * (never NULL). Other visitors leave *@obj unchanged, and commonly - * treat NULL like "". + * @obj must be non-NULL. Input and clone visitors set *@obj to the + * value (always using "" rather than NULL for an empty string). + * Other visitors leave *@obj unchanged, and commonly treat NULL like + * "". * * It is safe to cast away const when preparing a (const char *) value * into @obj for use by an output visitor. -- cgit v1.1 From 37f9e0a2b65a6dd5fe09cb0023b8001014aaaf01 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 9 Jun 2016 10:48:45 -0600 Subject: sockets: Use new QAPI cloning Rather than rolling our own clone via an expensive conversion in and back out of QObject, use the new clone visitor. Signed-off-by: Eric Blake Message-Id: <1465490926-28625-15-git-send-email-eblake@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- include/qapi/clone-visitor.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'include/qapi') diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h index 16ceff5..b16177e 100644 --- a/include/qapi/clone-visitor.h +++ b/include/qapi/clone-visitor.h @@ -11,7 +11,9 @@ #ifndef QAPI_CLONE_VISITOR_H #define QAPI_CLONE_VISITOR_H +#include "qemu/typedefs.h" #include "qapi/visitor.h" +#include "qapi-visit.h" /* * The clone visitor is for direct use only by the QAPI_CLONE() macro; -- cgit v1.1