| Message ID | 1461903820-3092-16-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > 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. > > Note that we can only clone objects (including alternates) and lists, > not built-ins. not built-ins or enums. > This is because of things like visit_type_uint8: our > visitor only implements a 64-bit callback, "Our visitor implements" suggests it's the clone visitor's fault. It's actually the visitor core's fault, and only since commit 04e070d. I guess we could clone everything except the integers narrower than 64 bits if we really wanted to. But all that would buy us now is more generated code we don't use. Suggest something like: 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 integers. Restricting cloning to just objects and lists is cleaner than restricting it to non-integers. > so we have no indication > what size int to read from the source, and cannot blindly assume that > it is safe to read a 64-bit int. As long as a built-in is not the > root of the visit, scalars copy over just fine by virtue of a > g_memdup() each time we push another struct onto the stack. > > As such, I tried to document that the clone visitor is for direct > use only by generated code; other code should stick to wrappers. > > 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 <eblake@redhat.com> qapi-types.h grows by 492 lines (19KiB, +19%). Roughly one non-blank line per non-simple type, including list types. It's included all over the place. qapi-types.c grows by 4212 lines (92KiB, +90%). Is it a good idea to generate all clone functions? As far as I can see, we use only a fraction of them. A sufficiently smart link-time optimizer can get rid of the ones we don't use. But I consider the "sufficiently smart optimizer" argument a cop out until proven otherwise. We already generate plenty of code we don't use, but that's not exactly a reason for generating more code we don't use. > > --- > v3: new patch > --- > include/qapi/visitor.h | 27 ++--- > include/qapi/visitor-impl.h | 1 + > scripts/qapi-types.py | 42 ++++++++ > include/qapi/clone-visitor.h | 28 +++++ > qapi/qapi-visit-core.c | 1 + > qapi/qapi-clone-visitor.c | 166 ++++++++++++++++++++++++++++++ > tests/test-clone-visitor.c | 239 +++++++++++++++++++++++++++++++++++++++++++ > docs/qapi-code-gen.txt | 38 +++++++ > qapi/Makefile.objs | 2 +- > tests/.gitignore | 1 + > tests/Makefile | 5 +- > 11 files changed, 536 insertions(+), 14 deletions(-) > create mode 100644 include/qapi/clone-visitor.h > create mode 100644 qapi/qapi-clone-visitor.c > create mode 100644 tests/test-clone-visitor.c > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index e8a4403..4c122cc 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -24,14 +24,15 @@ > * 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, string, and > * JSON) 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 > + * 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 > + * clone, string and QemuOpts 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 > @@ -85,16 +86,18 @@ > * struct. > * > * Additionally, in qapi-types.h, all QAPI pointer types (structs, > - * unions, alternates, and lists) have a generated function compatible > - * with: > + * unions, alternates, and lists) have two generated functions > + * compatible with: > * > + * FOO *qapi_FOO_clone(const FOO *src); > * 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 the former does a deep clone, and the latter behaves like > + * free() in that @obj may be NULL. Because of these functions, the > + * clone and dealloc visitor are 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: > * > * BASE *qapi_CHILD_base(CHILD *obj); > * > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 145afd0..a5a2dd0 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -35,6 +35,7 @@ typedef enum VisitorType { > VISITOR_INPUT, > VISITOR_OUTPUT, > VISITOR_DEALLOC, > + VISITOR_CLONE, It's *two* visitors, running in lockstep: an input visitor visiting @src, and an output visitor visiting the clone. To which one does the Visitor's type apply? Let's review how it's used: * visit_start_struct() if (obj && v->type == VISITOR_INPUT) { assert(!err != !*obj); } The clone visitor behaves like an input visitor here. * visit_start_alternate() Likewise. * visit_type_str() Likewise. * visit_type_any() Likewise. * visit_type_enum() if (v->type == VISITOR_INPUT) { input_type_enum(v, name, obj, strings, errp); } else if (v->type == VISITOR_OUTPUT) { output_type_enum(v, name, obj, strings, errp); } The clone visitor wants to override this completely. Hypothetical input from / output to binary visitors would probably want the same. Perhaps this part of commit da72ab0 wasn't a good idea. Overall, this looks like an input visitor to me so far. But let's have a look at its code. > } VisitorType; > > struct Visitor > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 437cf6c..c5ac493 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -116,6 +116,38 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) > c_name=c_name(name), base=base.c_name()) > > > +def gen_type_clone_decl(name): > + return mcgen(''' > + > +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src); Hmm. It's qapi_free_FOO(), but qapi_FOO_clone(). > +''', > + c_name=c_name(name)) > + > + > +def gen_type_clone(name): > + ret = mcgen(''' > + > +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src) > +{ > + QapiCloneVisitor *qcv; > + Visitor *v; > + %(c_name)s *dst; > + > + if (!src) { > + return NULL; > + } > + > + qcv = qapi_clone_visitor_new(src); > + v = qapi_clone_get_visitor(qcv); > + visit_type_%(c_name)s(v, NULL, &dst, &error_abort); > + qapi_clone_visitor_cleanup(qcv); > + return dst; > +} > +''', > + c_name=c_name(name)) > + return ret > + > + > def gen_variants(variants): > ret = mcgen(''' > union { /* union tag is @%(c_name)s */ > @@ -212,12 +244,16 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > if isinstance(element_type, QAPISchemaBuiltinType): > self._btin += gen_fwd_object_or_array(name) > self._btin += gen_array(name, element_type) > + self._btin += gen_type_clone_decl(name) > self._btin += gen_type_cleanup_decl(name) > if do_builtins: > + self.defn += gen_type_clone(name) > self.defn += gen_type_cleanup(name) > else: > self._fwdecl += gen_fwd_object_or_array(name) > self.decl += gen_array(name, element_type) > + self.decl += gen_type_clone_decl(name) > + self.defn += gen_type_clone(name) > self._gen_type_cleanup(name) > > def visit_object_type(self, name, info, base, members, variants): > @@ -232,11 +268,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > # directly use rather than repeat type.is_implicit()? > if not name.startswith('q_'): > # implicit types won't be directly allocated/freed > + self.decl += gen_type_clone_decl(name) > + self.defn += gen_type_clone(name) > self._gen_type_cleanup(name) > > def visit_alternate_type(self, name, info, variants): > self._fwdecl += gen_fwd_object_or_array(name) > self.decl += gen_object(name, None, [variants.tag_member], variants) > + self.decl += gen_type_clone_decl(name) > + self.defn += gen_type_clone(name) > self._gen_type_cleanup(name) > > # If you link code generated from multiple schemata, you want only one > @@ -288,7 +328,9 @@ h_comment = ''' > > fdef.write(mcgen(''' > #include "qemu/osdep.h" > +#include "qapi/clone-visitor.h" > #include "qapi/dealloc-visitor.h" > +#include "qapi/error.h" > #include "%(prefix)sqapi-types.h" > #include "%(prefix)sqapi-visit.h" > ''', > diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h > new file mode 100644 > index 0000000..8da5d0f > --- /dev/null > +++ b/include/qapi/clone-visitor.h > @@ -0,0 +1,28 @@ > +/* > + * Clone Visitor > + * > + * Copyright (C) 2016 Red Hat, Inc. > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > + * See the COPYING.LIB file in the top-level directory. > + * > + */ > + > +#ifndef QAPI_CLONE_VISITOR_H > +#define QAPI_CLONE_VISITOR_H > + > +#include "qapi/visitor.h" > + > +typedef struct QapiCloneVisitor QapiCloneVisitor; > + > +/* The clone visitor is for use only by generated qapi_FOO_clone() > + * functions; it requires that the root visit occur on an object, > + * list, or alternate, and is directly not usable on built-in QAPI > + * types. > + */ Wing your winged comments at both ends, please. > +QapiCloneVisitor *qapi_clone_visitor_new(const void *src); > +void qapi_clone_visitor_cleanup(QapiCloneVisitor *v); > + > +Visitor *qapi_clone_get_visitor(QapiCloneVisitor *v); > + > +#endif > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index f5d4b52..838e5d5 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -325,4 +325,5 @@ void visit_type_enum(Visitor *v, const char *name, int *obj, > } else if (v->type == VISITOR_OUTPUT) { > output_type_enum(v, name, obj, strings, errp); > } > + /* dealloc and clone visitors have nothing to do */ > } > diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c > new file mode 100644 > index 0000000..42384d3 > --- /dev/null > +++ b/qapi/qapi-clone-visitor.c > @@ -0,0 +1,166 @@ > +/* > + * Copy one QAPI object to another Uh, "copy" suggests it's an assignment. A clone is more. "Deep copy" would be better, but I'd say something like "Create a clone of a QAPI object". > + * > + * Copyright (C) 2016 Red Hat, Inc. > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. > + * See the COPYING.LIB file in the top-level directory. > + * > + */ > + > +#include "qemu/osdep.h" > +#include "qapi/clone-visitor.h" > +#include "qapi/visitor-impl.h" > + > +struct QapiCloneVisitor { > + Visitor visitor; > + const void *root; /* Must be object, alternate, or list */ > + size_t depth; > +}; > + > +static QapiCloneVisitor *to_qcv(Visitor *v) > +{ > + return container_of(v, QapiCloneVisitor, visitor); > +} > + > +static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, > + size_t size, Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + > + if (!obj) { > + /* Nothing to allocate on the virtual walk during an > + * alternate, but we still have to push depth. I don't quite get this comment. I understand why we don't memdup() --- it's pointless without a place to store the duplicate. I think I understand why we want to increment qcv->depth unconditionally, but I might be wrong. What exactly is the meaning of member depth? What's the relation to alternate? I see this is temporary. Should I not worry and move on? > + * FIXME: passing obj to visit_end_struct would make this easier */ > + assert(qcv->depth); > + qcv->depth++; > + return; > + } > + > + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); > + qcv->depth++; > +} > + > +static void qapi_clone_end(Visitor *v) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + qcv->depth--; > +} > + > +static void qapi_clone_start_list(Visitor *v, const char *name, > + GenericList **listp, size_t size, > + Error **errp) > +{ > + qapi_clone_start_struct(v, name, (void **)listp, size, errp); > +} > + > +static GenericList *qapi_clone_next_list(Visitor *v, GenericList *tail, > + size_t size) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Unshare the tail of the list cloned by g_memdup */ Humor me: g_memdup() > + tail->next = g_memdup(tail->next, size); > + return tail->next; > +} > + > +static void qapi_clone_start_alternate(Visitor *v, const char *name, > + GenericAlternate **obj, size_t size, > + bool promote_int, Error **errp) > +{ > + qapi_clone_start_struct(v, name, (void **)obj, size, errp); > +} > + > +static void qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj, > + Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Value was already cloned by g_memdup */ > +} > + > +static void qapi_clone_type_uint64(Visitor *v, const char *name, > + uint64_t *obj, Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Value was already cloned by g_memdup */ > +} > + > +static void qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, > + Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Value was already cloned by g_memdup */ > +} > + > +static void qapi_clone_type_str(Visitor *v, const char *name, char **obj, > + Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Pointer was already cloned by g_memdup; create fresh copy */ > + *obj = g_strdup(*obj); > +} > + > +static void qapi_clone_type_number(Visitor *v, const char *name, double *obj, > + Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Value was already cloned by g_memdup */ > +} > + > +static void qapi_clone_type_any(Visitor *v, const char *name, QObject **obj, > + Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Pointer was already copied by g_memdup; fix the refcount */ > + qobject_incref(*obj); 'any' values are shared? > +} > + > +static void qapi_clone_type_null(Visitor *v, const char *name, Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + assert(qcv->depth); > + /* Nothing to do */ > +} > + > +Visitor *qapi_clone_get_visitor(QapiCloneVisitor *v) > +{ > + return &v->visitor; > +} > + > +void qapi_clone_visitor_cleanup(QapiCloneVisitor *v) > +{ > + g_free(v); > +} > + > +QapiCloneVisitor *qapi_clone_visitor_new(const void *src) > +{ > + QapiCloneVisitor *v; > + > + v = g_malloc0(sizeof(*v)); > + v->root = src; > + > + v->visitor.type = VISITOR_CLONE; > + v->visitor.start_struct = qapi_clone_start_struct; > + v->visitor.end_struct = qapi_clone_end; > + v->visitor.start_list = qapi_clone_start_list; > + v->visitor.next_list = qapi_clone_next_list; > + v->visitor.end_list = qapi_clone_end; > + v->visitor.start_alternate = qapi_clone_start_alternate; > + v->visitor.end_alternate = qapi_clone_end; > + v->visitor.type_int64 = qapi_clone_type_int64; > + v->visitor.type_uint64 = qapi_clone_type_uint64; > + v->visitor.type_bool = qapi_clone_type_bool; > + v->visitor.type_str = qapi_clone_type_str; > + v->visitor.type_number = qapi_clone_type_number; > + v->visitor.type_any = qapi_clone_type_any; > + v->visitor.type_null = qapi_clone_type_null; > + > + return v; > +} [Getting late, skipping the test for now] > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index d7d6987..92fbb0e 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -787,6 +787,8 @@ Example: > char *string; > }; > > + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src); > + > void qapi_free_UserDefOne(UserDefOne *obj); > > struct UserDefOneList { > @@ -794,12 +796,31 @@ Example: > UserDefOne *value; > }; > > + UserDefOneList *qapi_UserDefOneList_clone(const UserDefOneList *src); > + > void qapi_free_UserDefOneList(UserDefOneList *obj); > > #endif > $ cat qapi-generated/example-qapi-types.c > [Uninteresting stuff omitted...] > > + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src) > + { > + QapiCloneVisitor *qcv; > + Visitor *v; > + UserDefOne *dst; Tab damage. More of the same below. > + > + if (!src) { > + return; > + } > + > + qcv = qapi_clone_visitor_new(src); > + v = qapi_clone_get_visitor(qcv); > + visit_type_UserDefOne(v, NULL, &dst, NULL); > + qapi_clone_visitor_cleanup(qcv); > + return dst; > + } > + > void qapi_free_UserDefOne(UserDefOne *obj) > { > QapiDeallocVisitor *qdv; > @@ -815,6 +836,23 @@ Example: > qapi_dealloc_visitor_cleanup(qdv); > } > > + UserDefOneList *qapi_UserDefOneList_clone(const UserDefOneList *src) > + { > + QapiCloneVisitor *qcv; > + Visitor *v; > + UserDefOneList *dst; > + > + if (!dst) { > + return; > + } > + > + qcv = qapi_clone_visitor_new(src); > + v = qapi_clone_get_visitor(qcv); > + visit_type_UserDefOneList(v, NULL, &dst, NULL); > + qapi_clone_visitor_cleanup(qcv); > + return dst; > + } > + > void qapi_free_UserDefOneList(UserDefOneList *obj) > { > QapiDeallocVisitor *qdv; > diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs > index b60e11b..1406df7 100644 > --- a/qapi/Makefile.objs > +++ b/qapi/Makefile.objs > @@ -1,6 +1,6 @@ > util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o > util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o > util-obj-y += string-input-visitor.o string-output-visitor.o > -util-obj-y += opts-visitor.o json-output-visitor.o > +util-obj-y += opts-visitor.o json-output-visitor.o qapi-clone-visitor.o > util-obj-y += qmp-event.o > util-obj-y += qapi-util.o > diff --git a/tests/.gitignore b/tests/.gitignore > index c2aad79..60ff7cc 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -12,6 +12,7 @@ test-aio > test-base64 > test-bitops > test-blockjob-txn > +test-clone-visitor > test-coroutine > test-crypto-afsplit > test-crypto-block > diff --git a/tests/Makefile b/tests/Makefile > index 1f8a39d..10ed072 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -24,6 +24,8 @@ check-unit-y += tests/test-qmp-output-visitor$(EXESUF) > gcov-files-test-qmp-output-visitor-y = qapi/qmp-output-visitor.c > check-unit-y += tests/test-json-output-visitor$(EXESUF) > gcov-files-test-json-output-visitor-y = qapi/json-output-visitor.c > +check-unit-y += tests/test-clone-visitor$(EXESUF) > +gcov-files-test-clone-visitor-y = qapi/qapi-clone-visitor.c > check-unit-y += tests/test-qmp-input-visitor$(EXESUF) > gcov-files-test-qmp-input-visitor-y = qapi/qmp-input-visitor.c > check-unit-y += tests/test-qmp-input-strict$(EXESUF) > @@ -390,7 +392,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ > tests/check-qobject-json.o \ > tests/test-coroutine.o tests/test-string-output-visitor.o \ > tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ > - tests/test-json-output-visitor.o \ > + tests/test-clone-visitor.o tests/test-json-output-visitor.o \ > tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ > tests/test-qmp-commands.o tests/test-visitor-serialization.o \ > tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \ > @@ -482,6 +484,7 @@ tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(te > tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) > tests/test-qmp-output-visitor$(EXESUF): tests/test-qmp-output-visitor.o $(test-qapi-obj-y) > tests/test-json-output-visitor$(EXESUF): tests/test-json-output-visitor.o $(test-qapi-obj-y) > +tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y) > tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qapi-obj-y) > tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y) > tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
On 05/02/2016 11:54 AM, Markus Armbruster wrote: > > qapi-types.h grows by 492 lines (19KiB, +19%). Roughly one non-blank > line per non-simple type, including list types. It's included all over > the place. > > qapi-types.c grows by 4212 lines (92KiB, +90%). > > Is it a good idea to generate all clone functions? As far as I can see, > we use only a fraction of them. Would it be worth creating a separate .h file, and only include that file in the few places that actually want a clone, rather than making every file pay the price of a larger .h? > > A sufficiently smart link-time optimizer can get rid of the ones we > don't use. But I consider the "sufficiently smart optimizer" argument a > cop out until proven otherwise. We could also teach the qapi generator to mark specific types as cloneable (and then the transitive closure of all subtypes included by those types are also cloneable), and only generate the clone functions where it matters, rather than for every type. I can play with that idea (basically, adding a 'clone':true flag in the .json file, then figuring out how to propagate that through all subtypes). > > We already generate plenty of code we don't use, but that's not exactly > a reason for generating more code we don't use. I guess it's a trade-off of whether we will find new uses rather than the two immediate places that I fix in the next couple of patches - if we need more types to be cloneable, how much maintenance effort is it to mark those additional types cloneable? >> +++ b/include/qapi/visitor-impl.h >> @@ -35,6 +35,7 @@ typedef enum VisitorType { >> VISITOR_INPUT, >> VISITOR_OUTPUT, >> VISITOR_DEALLOC, >> + VISITOR_CLONE, > > It's *two* visitors, running in lockstep: an input visitor visiting > @src, and an output visitor visiting the clone. > > To which one does the Visitor's type apply? > > Let's review how it's used: > > * visit_start_struct() > > if (obj && v->type == VISITOR_INPUT) { > assert(!err != !*obj); > } > > The clone visitor behaves like an input visitor here. It doesn't actually set err, but is indeed allocating *obj. > * visit_type_enum() > > if (v->type == VISITOR_INPUT) { > input_type_enum(v, name, obj, strings, errp); > } else if (v->type == VISITOR_OUTPUT) { > output_type_enum(v, name, obj, strings, errp); > } > > The clone visitor wants to override this completely. The override is to have visit_type_enum() be a no-op (because we don't visit top-level enums, and an enum which is a member of a struct or list is cloned as part of the memdup() of pushing into the struct or list). The dealloc visitor also has visit_type_enum() be a no-op. > > Hypothetical input from / output to binary visitors would probably > want the same. Perhaps this part of commit da72ab0 wasn't a good > idea. The clone visitor is cloning C structs, not other representations (that is, this is NOT a QObject cloner, and therefore even if we add hypothetical binary representation input/output visitors, this wil NOT be cloning those binary representations). The fact that we now have a fourth category of visitor, where dealloc and clone visitors behave the same for visit_type_enum, didn't seem too bad. >> +def gen_type_clone_decl(name): >> + return mcgen(''' >> + >> +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src); > > Hmm. It's qapi_free_FOO(), but qapi_FOO_clone(). and qapi_visit_FOO_members(). I'm fine swapping names to whatever you find nicer, but think qapi_clone_FOO() is probably best now that you mention it. >> +static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, >> + size_t size, Error **errp) >> +{ >> + QapiCloneVisitor *qcv = to_qcv(v); >> + >> + if (!obj) { >> + /* Nothing to allocate on the virtual walk during an >> + * alternate, but we still have to push depth. > > I don't quite get this comment. I understand why we don't memdup() --- > it's pointless without a place to store the duplicate. I think I > understand why we want to increment qcv->depth unconditionally, but I > might be wrong. What exactly is the meaning of member depth? > > What's the relation to alternate? > > I see this is temporary. Should I not worry and move on? I can still make the comment better. Basically, when we are doing visit_type_ALTERNATE() and happen to pick the object branch of the alternate, we call visit_start_struct(NULL) under the hood; the 'if (!obj)' should NOT be reachable for a top-level visit (because we state that this visitor is limited and cannot do virtual walks), but it IS reachable under the hood when walking an alternate. So maybe the comment should read: if (!obj) { /* * Reachable only when visiting an alternate. Nothing to allocate, * and visit_start_alternate() already copied memory, so we have * nothing further to do except increment depth for proper * bookkeeping during visit_end_struct(). */ > >> + * FIXME: passing obj to visit_end_struct would make this easier */ >> + assert(qcv->depth); >> + qcv->depth++; >> + return; >> + } >> + >> + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); >> + qcv->depth++; >> +} >> +static void qapi_clone_type_any(Visitor *v, const char *name, QObject **obj, >> + Error **errp) >> +{ >> + QapiCloneVisitor *qcv = to_qcv(v); >> + assert(qcv->depth); >> + /* Pointer was already copied by g_memdup; fix the refcount */ >> + qobject_incref(*obj); > > 'any' values are shared? Unless you know of a way to deep clone QObject, and a reason that a deep clone is better than sharing. The reason we have to deep clone the rest of the QAPI object is that QAPI doesn't have ref-counting the way QObject does. >> + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src) >> + { >> + QapiCloneVisitor *qcv; >> + Visitor *v; >> + UserDefOne *dst; > > Tab damage. More of the same below. I can't make emacs figure out that the docs file doesn't need TABs, so it's not the first time I've tab-damaged a patch (although sometimes I've managed to catch it before leaking it to the list - not this time, though).
Eric Blake <eblake@redhat.com> writes: > On 05/02/2016 11:54 AM, Markus Armbruster wrote: > >> >> qapi-types.h grows by 492 lines (19KiB, +19%). Roughly one non-blank >> line per non-simple type, including list types. It's included all over >> the place. >> >> qapi-types.c grows by 4212 lines (92KiB, +90%). >> >> Is it a good idea to generate all clone functions? As far as I can see, >> we use only a fraction of them. > > Would it be worth creating a separate .h file, and only include that > file in the few places that actually want a clone, rather than making > every file pay the price of a larger .h? Possibly, but it might be a bother, as the generator scripts and Makefiles are designed for one .h + one .c. >> A sufficiently smart link-time optimizer can get rid of the ones we >> don't use. But I consider the "sufficiently smart optimizer" argument a >> cop out until proven otherwise. > > We could also teach the qapi generator to mark specific types as > cloneable (and then the transitive closure of all subtypes included by > those types are also cloneable), and only generate the clone functions > where it matters, rather than for every type. I can play with that idea > (basically, adding a 'clone':true flag in the .json file, then figuring > out how to propagate that through all subtypes). Related: generation of list types. We got away with DummyForceArrays there. I don't think you need the transitive closure: the generated clone function doesn't recurse into generated clone functions, it sets up recursion with the clone visitor. Taking a step back: the generated clone functions feel like a case of generitis. Here's their template: %(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src) { QapiCloneVisitor *qcv; Visitor *v; %(c_name)s *dst; if (!src) { return NULL; } qcv = qapi_clone_visitor_new(src); v = qapi_clone_get_visitor(qcv); visit_type_%(c_name)s(v, NULL, &dst, &error_abort); qapi_clone_visitor_cleanup(qcv); return dst; } The job could be done by a single function: void *qapi_clone(const void *src, void (*visit_type)(Visitor, const char *, void **, Error **)) { QapiCloneVisitor *qcv; Visitor *v; void *dst; if (!src) { return NULL; } qcv = qapi_clone_visitor_new(src); v = qapi_clone_get_visitor(qcv); visit_type(v, NULL, &dst, &error_abort); qapi_clone_visitor_cleanup(qcv); return dst; } However, passing a visit_type_FOO to this function requires a type cast. Type-punning functions is always ugly. A macro would do the job without the type punning. Inline: #define QAPI_CLONE(type, src) \ (src ? ({ \ QapiCloneVisitor *qcv; \ Visitor *v; \ type *dst; \ \ qcv = qapi_clone_visitor_new(src); \ v = qapi_clone_get_visitor(qcv); \ visit_type_ ## type(v, NULL, &dst, &error_abort); \ qapi_clone_visitor_cleanup(qcv); \ dst; \ }) \ : NULL) Out of line: #define QAPI_DEFINE_CLONE(type, src) \ type *qapi_ ## type ## _clone(const type *src) \ { \ QapiCloneVisitor *qcv; \ Visitor *v; \ type *dst; \ \ if (!src) { \ return NULL; \ } \ \ qcv = qapi_clone_visitor_new(src); \ v = qapi_clone_get_visitor(qcv); \ visit_type_ ## type(v, NULL, &dst, &error_abort); \ qapi_clone_visitor_cleanup(qcv); \ return dst; \ } where non-generated code uses QAPI_DEFINE_CLONE() to define the ones we actually need. Differently ugly. Note that the qapi_free_FOO() are similar. They're used more frequently, which makes the inline macro less attractive. >> We already generate plenty of code we don't use, but that's not exactly >> a reason for generating more code we don't use. > > I guess it's a trade-off of whether we will find new uses rather than > the two immediate places that I fix in the next couple of patches - if > we need more types to be cloneable, how much maintenance effort is it to > mark those additional types cloneable? Compared to writing new code that needs a clone, adding one more line to get the clone function is nothing. The time needed to realize you need to do it and to figure out how to do it may not be nothing. Documentation problem, I'd say. >>> +++ b/include/qapi/visitor-impl.h >>> @@ -35,6 +35,7 @@ typedef enum VisitorType { >>> VISITOR_INPUT, >>> VISITOR_OUTPUT, >>> VISITOR_DEALLOC, >>> + VISITOR_CLONE, >> >> It's *two* visitors, running in lockstep: an input visitor visiting >> @src, and an output visitor visiting the clone. >> >> To which one does the Visitor's type apply? >> >> Let's review how it's used: >> >> * visit_start_struct() >> >> if (obj && v->type == VISITOR_INPUT) { >> assert(!err != !*obj); >> } >> >> The clone visitor behaves like an input visitor here. > > It doesn't actually set err, but is indeed allocating *obj. If it could fail, it would set err then. Followup question: should we assert this for a clone visitor, too? >> * visit_type_enum() >> >> if (v->type == VISITOR_INPUT) { >> input_type_enum(v, name, obj, strings, errp); >> } else if (v->type == VISITOR_OUTPUT) { >> output_type_enum(v, name, obj, strings, errp); >> } >> >> The clone visitor wants to override this completely. > > The override is to have visit_type_enum() be a no-op (because we don't > visit top-level enums, and an enum which is a member of a struct or list > is cloned as part of the memdup() of pushing into the struct or list). If we still had the type_enum() method removed in commit da72ab0, we'd do enums exactly like other scalar types: static void qapi_clone_type_enum(Visitor *v, const char *name, int *obj, const char *const strings[], Error **errp) { QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); /* Value was already cloned by g_memdup() */ } > The dealloc visitor also has visit_type_enum() be a no-op. The dealloc visitor could be viewed as output visitor with a relaxed precondition: incompletely constructed input is okay. Anyway, it's semantically special enough to require its own clauses in the visitor documentation, so making its own visitor type is justifiable. >> Hypothetical input from / output to binary visitors would probably >> want the same. Perhaps this part of commit da72ab0 wasn't a good >> idea. > > The clone visitor is cloning C structs, not other representations (that > is, this is NOT a QObject cloner, and therefore even if we add > hypothetical binary representation input/output visitors, this wil NOT > be cloning those binary representations). Of course. But I was trying to say something else. Let me try again. Consider the five visitors QMP input, string input, JSON input (hypothetical), binary input (hypothetical), clone. All five build a tree of QAPI objects guided by some (visitor-specific) input. Visitor-specific means the visitor core is oblivious of the kind of input. For the QMP input visitor, the input is a QObject. For the string input visitor, the input is a string in idiosyncratic syntax. For the hypothetical JSON input visitor, the input would be an RFC 7159 JSON-text. For the hypothetical binary input visitor, the input would be a bunch of bytes. For the clone visitor, the input is another tree of QAPI objects. My point is: for the visitor core, there is no difference between these five. This separation of concerns is only proper. Except for one place, where we let a visitor detail bleed into the core: enums. The bleeding happened in commit da72ab0. We did it because it de-duplicated > The fact that we now have a > fourth category of visitor, where dealloc and clone visitors behave the > same for visit_type_enum, didn't seem too bad. Oh, it's certainly not *bad*. I'm just wondering whether it's necessary, and whether the existing conditionals on v->type need updating. I also hope to gain a better understanding of it in the process. >>> +def gen_type_clone_decl(name): >>> + return mcgen(''' >>> + >>> +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src); >> >> Hmm. It's qapi_free_FOO(), but qapi_FOO_clone(). > > and qapi_visit_FOO_members(). I'm fine swapping names to whatever you > find nicer, but think qapi_clone_FOO() is probably best now that you > mention it. > > >>> +static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, >>> + size_t size, Error **errp) >>> +{ >>> + QapiCloneVisitor *qcv = to_qcv(v); >>> + >>> + if (!obj) { >>> + /* Nothing to allocate on the virtual walk during an >>> + * alternate, but we still have to push depth. >> >> I don't quite get this comment. I understand why we don't memdup() --- >> it's pointless without a place to store the duplicate. I think I >> understand why we want to increment qcv->depth unconditionally, but I >> might be wrong. What exactly is the meaning of member depth? >> >> What's the relation to alternate? >> >> I see this is temporary. Should I not worry and move on? > > I can still make the comment better. > > Basically, when we are doing visit_type_ALTERNATE() and happen to pick > the object branch of the alternate, we call visit_start_struct(NULL) > under the hood; the 'if (!obj)' should NOT be reachable for a top-level > visit (because we state that this visitor is limited and cannot do > virtual walks), but it IS reachable under the hood when walking an > alternate. Right... > So maybe the comment should read: > > if (!obj) { > /* > * Reachable only when visiting an alternate. Nothing to allocate, an alternate's object branch > * and visit_start_alternate() already copied memory, so we have > * nothing further to do except increment depth for proper > * bookkeeping during visit_end_struct(). > */ > >> >>> + * FIXME: passing obj to visit_end_struct would make this easier */ >>> + assert(qcv->depth); >>> + qcv->depth++; >>> + return; >>> + } >>> + >>> + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); >>> + qcv->depth++; >>> +} > >>> +static void qapi_clone_type_any(Visitor *v, const char *name, QObject **obj, >>> + Error **errp) >>> +{ >>> + QapiCloneVisitor *qcv = to_qcv(v); >>> + assert(qcv->depth); >>> + /* Pointer was already copied by g_memdup; fix the refcount */ >>> + qobject_incref(*obj); >> >> 'any' values are shared? > > Unless you know of a way to deep clone QObject, and a reason that a deep > clone is better than sharing. The reason we have to deep clone the rest > of the QAPI object is that QAPI doesn't have ref-counting the way > QObject does. Simplifying management of the object life times is one reason to clone. The other, more important one is to avoid unwanted sharing of modifications. Only applies for mutable data, of course. So yes, life time management is one reason for deep cloning QAPI, and it doesn't apply to the 'any' values. But the other reason does apply, since 'any' values can be mutable. Options: (1) Document 'any' values are shared between the clone and the original. Dangerous trap, if you ask me. (2) Don't implement type_any(). Attempts to clone something containing 'any' values crash. Document that as a restriction. Ugly: we can generate clone functions that cannot be used. This leads to: (3) Like (2), but additionally refrain from generating clone functions that can crash. More restrive than (2), because with (2) such a clone function still works when the 'any' values are optional and absent, including empty ['any']. (4) Implement QObject cloning. >>> + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src) >>> + { >>> + QapiCloneVisitor *qcv; >>> + Visitor *v; >>> + UserDefOne *dst; >> >> Tab damage. More of the same below. > > I can't make emacs figure out that the docs file doesn't need TABs, so > it's not the first time I've tab-damaged a patch (although sometimes > I've managed to catch it before leaking it to the list - not this time, > though). We can't disable indent-tabs-mode for all files in .dir-locals.el, because that would wreak havoc with Makefiles. We could try disabling it for text-mode. If we're bold, we could try disabling it for everything, then enable it for makefile-mode.
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index e8a4403..4c122cc 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -24,14 +24,15 @@ * 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, string, and * JSON) 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 + * 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 + * clone, string and QemuOpts 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 @@ -85,16 +86,18 @@ * struct. * * Additionally, in qapi-types.h, all QAPI pointer types (structs, - * unions, alternates, and lists) have a generated function compatible - * with: + * unions, alternates, and lists) have two generated functions + * compatible with: * + * FOO *qapi_FOO_clone(const FOO *src); * 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 the former does a deep clone, and the latter behaves like + * free() in that @obj may be NULL. Because of these functions, the + * clone and dealloc visitor are 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: * * BASE *qapi_CHILD_base(CHILD *obj); * diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 145afd0..a5a2dd0 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -35,6 +35,7 @@ typedef enum VisitorType { VISITOR_INPUT, VISITOR_OUTPUT, VISITOR_DEALLOC, + VISITOR_CLONE, } VisitorType; struct Visitor diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 437cf6c..c5ac493 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,6 +116,38 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) c_name=c_name(name), base=base.c_name()) +def gen_type_clone_decl(name): + return mcgen(''' + +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src); +''', + c_name=c_name(name)) + + +def gen_type_clone(name): + ret = mcgen(''' + +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src) +{ + QapiCloneVisitor *qcv; + Visitor *v; + %(c_name)s *dst; + + if (!src) { + return NULL; + } + + qcv = qapi_clone_visitor_new(src); + v = qapi_clone_get_visitor(qcv); + visit_type_%(c_name)s(v, NULL, &dst, &error_abort); + qapi_clone_visitor_cleanup(qcv); + return dst; +} +''', + c_name=c_name(name)) + return ret + + def gen_variants(variants): ret = mcgen(''' union { /* union tag is @%(c_name)s */ @@ -212,12 +244,16 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): if isinstance(element_type, QAPISchemaBuiltinType): self._btin += gen_fwd_object_or_array(name) self._btin += gen_array(name, element_type) + self._btin += gen_type_clone_decl(name) self._btin += gen_type_cleanup_decl(name) if do_builtins: + self.defn += gen_type_clone(name) self.defn += gen_type_cleanup(name) else: self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_array(name, element_type) + self.decl += gen_type_clone_decl(name) + self.defn += gen_type_clone(name) self._gen_type_cleanup(name) def visit_object_type(self, name, info, base, members, variants): @@ -232,11 +268,15 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): # directly use rather than repeat type.is_implicit()? if not name.startswith('q_'): # implicit types won't be directly allocated/freed + self.decl += gen_type_clone_decl(name) + self.defn += gen_type_clone(name) self._gen_type_cleanup(name) def visit_alternate_type(self, name, info, variants): self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_object(name, None, [variants.tag_member], variants) + self.decl += gen_type_clone_decl(name) + self.defn += gen_type_clone(name) self._gen_type_cleanup(name) # If you link code generated from multiple schemata, you want only one @@ -288,7 +328,9 @@ h_comment = ''' fdef.write(mcgen(''' #include "qemu/osdep.h" +#include "qapi/clone-visitor.h" #include "qapi/dealloc-visitor.h" +#include "qapi/error.h" #include "%(prefix)sqapi-types.h" #include "%(prefix)sqapi-visit.h" ''', diff --git a/include/qapi/clone-visitor.h b/include/qapi/clone-visitor.h new file mode 100644 index 0000000..8da5d0f --- /dev/null +++ b/include/qapi/clone-visitor.h @@ -0,0 +1,28 @@ +/* + * Clone Visitor + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#ifndef QAPI_CLONE_VISITOR_H +#define QAPI_CLONE_VISITOR_H + +#include "qapi/visitor.h" + +typedef struct QapiCloneVisitor QapiCloneVisitor; + +/* The clone visitor is for use only by generated qapi_FOO_clone() + * functions; it requires that the root visit occur on an object, + * list, or alternate, and is directly not usable on built-in QAPI + * types. + */ +QapiCloneVisitor *qapi_clone_visitor_new(const void *src); +void qapi_clone_visitor_cleanup(QapiCloneVisitor *v); + +Visitor *qapi_clone_get_visitor(QapiCloneVisitor *v); + +#endif diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index f5d4b52..838e5d5 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -325,4 +325,5 @@ void visit_type_enum(Visitor *v, const char *name, int *obj, } else if (v->type == VISITOR_OUTPUT) { output_type_enum(v, name, obj, strings, errp); } + /* dealloc and clone visitors have nothing to do */ } diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c new file mode 100644 index 0000000..42384d3 --- /dev/null +++ b/qapi/qapi-clone-visitor.c @@ -0,0 +1,166 @@ +/* + * Copy one QAPI object to another + * + * Copyright (C) 2016 Red Hat, Inc. + * + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later. + * See the COPYING.LIB file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "qapi/clone-visitor.h" +#include "qapi/visitor-impl.h" + +struct QapiCloneVisitor { + Visitor visitor; + const void *root; /* Must be object, alternate, or list */ + size_t depth; +}; + +static QapiCloneVisitor *to_qcv(Visitor *v) +{ + return container_of(v, QapiCloneVisitor, visitor); +} + +static void qapi_clone_start_struct(Visitor *v, const char *name, void **obj, + size_t size, Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + + if (!obj) { + /* Nothing to allocate on the virtual walk during an + * alternate, but we still have to push depth. + * FIXME: passing obj to visit_end_struct would make this easier */ + assert(qcv->depth); + qcv->depth++; + return; + } + + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); + qcv->depth++; +} + +static void qapi_clone_end(Visitor *v) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + qcv->depth--; +} + +static void qapi_clone_start_list(Visitor *v, const char *name, + GenericList **listp, size_t size, + Error **errp) +{ + qapi_clone_start_struct(v, name, (void **)listp, size, errp); +} + +static GenericList *qapi_clone_next_list(Visitor *v, GenericList *tail, + size_t size) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Unshare the tail of the list cloned by g_memdup */ + tail->next = g_memdup(tail->next, size); + return tail->next; +} + +static void qapi_clone_start_alternate(Visitor *v, const char *name, + GenericAlternate **obj, size_t size, + bool promote_int, Error **errp) +{ + qapi_clone_start_struct(v, name, (void **)obj, size, errp); +} + +static void qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj, + Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Value was already cloned by g_memdup */ +} + +static void qapi_clone_type_uint64(Visitor *v, const char *name, + uint64_t *obj, Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Value was already cloned by g_memdup */ +} + +static void qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, + Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Value was already cloned by g_memdup */ +} + +static void qapi_clone_type_str(Visitor *v, const char *name, char **obj, + Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Pointer was already cloned by g_memdup; create fresh copy */ + *obj = g_strdup(*obj); +} + +static void qapi_clone_type_number(Visitor *v, const char *name, double *obj, + Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Value was already cloned by g_memdup */ +} + +static void qapi_clone_type_any(Visitor *v, const char *name, QObject **obj, + Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Pointer was already copied by g_memdup; fix the refcount */ + qobject_incref(*obj); +} + +static void qapi_clone_type_null(Visitor *v, const char *name, Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + assert(qcv->depth); + /* Nothing to do */ +} + +Visitor *qapi_clone_get_visitor(QapiCloneVisitor *v) +{ + return &v->visitor; +} + +void qapi_clone_visitor_cleanup(QapiCloneVisitor *v) +{ + g_free(v); +} + +QapiCloneVisitor *qapi_clone_visitor_new(const void *src) +{ + QapiCloneVisitor *v; + + v = g_malloc0(sizeof(*v)); + v->root = src; + + v->visitor.type = VISITOR_CLONE; + v->visitor.start_struct = qapi_clone_start_struct; + v->visitor.end_struct = qapi_clone_end; + v->visitor.start_list = qapi_clone_start_list; + v->visitor.next_list = qapi_clone_next_list; + v->visitor.end_list = qapi_clone_end; + v->visitor.start_alternate = qapi_clone_start_alternate; + v->visitor.end_alternate = qapi_clone_end; + v->visitor.type_int64 = qapi_clone_type_int64; + v->visitor.type_uint64 = qapi_clone_type_uint64; + v->visitor.type_bool = qapi_clone_type_bool; + v->visitor.type_str = qapi_clone_type_str; + v->visitor.type_number = qapi_clone_type_number; + v->visitor.type_any = qapi_clone_type_any; + v->visitor.type_null = qapi_clone_type_null; + + return v; +} diff --git a/tests/test-clone-visitor.c b/tests/test-clone-visitor.c new file mode 100644 index 0000000..4aeaece --- /dev/null +++ b/tests/test-clone-visitor.c @@ -0,0 +1,239 @@ +/* + * QAPI Clone Visitor unit-tests. + * + * 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. + */ + +#include "qemu/osdep.h" +#include <glib.h> + +#include "qemu-common.h" +#include "qapi/clone-visitor.h" +#include "test-qapi-types.h" +#include "test-qapi-visit.h" +#include "qapi/qmp/types.h" + +static void test_clone_struct(void) +{ + UserDefOne *src, *dst; + + src = g_new0(UserDefOne, 1); + src->integer = 42; + src->string = g_strdup("Hello"); + src->has_enum1 = false; + src->enum1 = ENUM_ONE_VALUE2; + + dst = qapi_UserDefOne_clone(src); + g_assert(dst); + g_assert_cmpint(dst->integer, ==, 42); + g_assert(dst->string != src->string); + g_assert_cmpstr(dst->string, ==, "Hello"); + g_assert_cmpint(dst->has_enum1, ==, false); + /* Our implementation does this, but it is not required: + g_assert_cmpint(dst->enum1, ==, ENUM_ONE_VALUE2); + */ + + qapi_free_UserDefOne(src); + qapi_free_UserDefOne(dst); +} + +static void test_clone_alternate(void) +{ + AltStrBool *b_src, *s_src, *b_dst, *s_dst; + + b_src = g_new0(AltStrBool, 1); + b_src->type = QTYPE_QBOOL; + b_src->u.b = true; + s_src = g_new0(AltStrBool, 1); + s_src->type = QTYPE_QSTRING; + s_src->u.s = g_strdup("World"); + + b_dst = qapi_AltStrBool_clone(b_src); + g_assert(b_dst); + g_assert_cmpint(b_dst->type, ==, b_src->type); + g_assert_cmpint(b_dst->u.b, ==, b_src->u.b); + s_dst = qapi_AltStrBool_clone(s_src); + g_assert(s_dst); + g_assert_cmpint(s_dst->type, ==, s_src->type); + g_assert_cmpstr(s_dst->u.s, ==, s_src->u.s); + g_assert(s_dst->u.s != s_src->u.s); + + qapi_free_AltStrBool(b_src); + qapi_free_AltStrBool(s_src); + qapi_free_AltStrBool(b_dst); + qapi_free_AltStrBool(s_dst); +} + +static void test_clone_native_list(void) +{ + uint8List *src, *dst; + uint8List *tmp = NULL; + int i; + + /* Build list in reverse */ + for (i = 10; i; i--) { + src = g_new0(uint8List, 1); + src->next = tmp; + src->value = i; + tmp = src; + } + + dst = qapi_uint8List_clone(src); + for (tmp = dst, i = 1; i <= 10; i++) { + g_assert(tmp); + g_assert_cmpint(tmp->value, ==, i); + tmp = tmp->next; + } + g_assert(!tmp); + + qapi_free_uint8List(src); + qapi_free_uint8List(dst); +} + +static void test_clone_empty(void) +{ + Empty2 *src, *dst; + + src = g_new0(Empty2, 1); + dst = qapi_Empty2_clone(src); + g_assert(dst); + qapi_free_Empty2(src); + qapi_free_Empty2(dst); +} + +static void test_clone_complex1(void) +{ + UserDefNativeListUnion *src, *dst; + anyList *tmp; + QDict *dict; + QList *list; + + src = g_new0(UserDefNativeListUnion, 1); + src->type = USER_DEF_NATIVE_LIST_UNION_KIND_STRING; + + dst = qapi_UserDefNativeListUnion_clone(src); + g_assert(dst); + g_assert_cmpint(dst->type, ==, src->type); + g_assert(!dst->u.string.data); + qapi_free_UserDefNativeListUnion(dst); + + src->type = USER_DEF_NATIVE_LIST_UNION_KIND_ANY; + tmp = src->u.any.data = g_new0(anyList, 1); + tmp->value = QOBJECT(qint_from_int(42)); + tmp = tmp->next = g_new0(anyList, 1); + tmp->value = QOBJECT(dict = qdict_new()); + qdict_put(dict, "key", qstring_from_str("value")); + tmp = tmp->next = g_new0(anyList, 1); + tmp->value = QOBJECT(qlist_new()); + + dst = qapi_UserDefNativeListUnion_clone(src); + g_assert(dst); + g_assert_cmpint(dst->type, ==, src->type); + tmp = dst->u.any.data; + g_assert(tmp); + g_assert_cmpint(qobject_type(tmp->value), ==, QTYPE_QINT); + g_assert_cmpint(qint_get_int(qobject_to_qint(tmp->value)), ==, 42); + tmp = tmp->next; + g_assert(tmp); + g_assert_cmpint(qobject_type(tmp->value), ==, QTYPE_QDICT); + dict = qobject_to_qdict(tmp->value); + g_assert_cmpint(qdict_size(dict), ==, 1); + g_assert_cmpstr(qdict_get_str(dict, "key"), ==, "value"); + tmp = tmp->next; + g_assert(tmp); + g_assert_cmpint(qobject_type(tmp->value), ==, QTYPE_QLIST); + list = qobject_to_qlist(tmp->value); + g_assert_cmpint(qlist_size(list), ==, 0); + tmp = tmp->next; + g_assert(!tmp); + + qapi_free_UserDefNativeListUnion(src); + qapi_free_UserDefNativeListUnion(dst); +} + +static void test_clone_complex2(void) +{ + WrapAlternate *src, *dst; + + src = g_new0(WrapAlternate, 1); + src->alt = g_new(UserDefAlternate, 1); + src->alt->type = QTYPE_QDICT; + src->alt->u.udfu.integer = 42; + src->alt->u.udfu.string = g_strdup("Hello"); + src->alt->u.udfu.enum1 = ENUM_ONE_VALUE3; + src->alt->u.udfu.u.value3.intb = 99; + src->alt->u.udfu.u.value3.has_a_b = true; + src->alt->u.udfu.u.value3.a_b = true; + + dst = qapi_WrapAlternate_clone(src); + g_assert(dst); + g_assert(dst->alt); + g_assert_cmpint(dst->alt->type, ==, QTYPE_QDICT); + g_assert_cmpint(dst->alt->u.udfu.integer, ==, 42); + g_assert_cmpstr(dst->alt->u.udfu.string, ==, "Hello"); + g_assert_cmpint(dst->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE3); + g_assert_cmpint(dst->alt->u.udfu.u.value3.intb, ==, 99); + g_assert_cmpint(dst->alt->u.udfu.u.value3.has_a_b, ==, true); + g_assert_cmpint(dst->alt->u.udfu.u.value3.a_b, ==, true); + + qapi_free_WrapAlternate(src); + qapi_free_WrapAlternate(dst); +} + +static void test_clone_complex3(void) +{ + __org_qemu_x_Struct2 *src, *dst; + __org_qemu_x_Union1List *tmp; + + src = g_new0(__org_qemu_x_Struct2, 1); + tmp = src->array = g_new0(__org_qemu_x_Union1List, 1); + tmp->value = g_new0(__org_qemu_x_Union1, 1); + tmp->value->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH; + tmp->value->u.__org_qemu_x_branch.data = g_strdup("one"); + tmp = tmp->next = g_new0(__org_qemu_x_Union1List, 1); + tmp->value = g_new0(__org_qemu_x_Union1, 1); + tmp->value->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH; + tmp->value->u.__org_qemu_x_branch.data = g_strdup("two"); + tmp = tmp->next = g_new0(__org_qemu_x_Union1List, 1); + tmp->value = g_new0(__org_qemu_x_Union1, 1); + tmp->value->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH; + tmp->value->u.__org_qemu_x_branch.data = g_strdup("three"); + + dst = qapi___org_qemu_x_Struct2_clone(src); + g_assert(dst); + tmp = dst->array; + g_assert(tmp); + g_assert(tmp->value); + g_assert_cmpstr(tmp->value->u.__org_qemu_x_branch.data, ==, "one"); + tmp = tmp->next; + g_assert(tmp); + g_assert(tmp->value); + g_assert_cmpstr(tmp->value->u.__org_qemu_x_branch.data, ==, "two"); + tmp = tmp->next; + g_assert(tmp); + g_assert(tmp->value); + g_assert_cmpstr(tmp->value->u.__org_qemu_x_branch.data, ==, "three"); + tmp = tmp->next; + g_assert(!tmp); + + qapi_free___org_qemu_x_Struct2(src); + qapi_free___org_qemu_x_Struct2(dst); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/visitor/clone/struct", test_clone_struct); + g_test_add_func("/visitor/clone/alternate", test_clone_alternate); + g_test_add_func("/visitor/clone/native_list", test_clone_native_list); + g_test_add_func("/visitor/clone/empty", test_clone_empty); + g_test_add_func("/visitor/clone/complex1", test_clone_complex1); + g_test_add_func("/visitor/clone/complex2", test_clone_complex2); + g_test_add_func("/visitor/clone/complex3", test_clone_complex3); + + return g_test_run(); +} diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index d7d6987..92fbb0e 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -787,6 +787,8 @@ Example: char *string; }; + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src); + void qapi_free_UserDefOne(UserDefOne *obj); struct UserDefOneList { @@ -794,12 +796,31 @@ Example: UserDefOne *value; }; + UserDefOneList *qapi_UserDefOneList_clone(const UserDefOneList *src); + void qapi_free_UserDefOneList(UserDefOneList *obj); #endif $ cat qapi-generated/example-qapi-types.c [Uninteresting stuff omitted...] + UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src) + { + QapiCloneVisitor *qcv; + Visitor *v; + UserDefOne *dst; + + if (!src) { + return; + } + + qcv = qapi_clone_visitor_new(src); + v = qapi_clone_get_visitor(qcv); + visit_type_UserDefOne(v, NULL, &dst, NULL); + qapi_clone_visitor_cleanup(qcv); + return dst; + } + void qapi_free_UserDefOne(UserDefOne *obj) { QapiDeallocVisitor *qdv; @@ -815,6 +836,23 @@ Example: qapi_dealloc_visitor_cleanup(qdv); } + UserDefOneList *qapi_UserDefOneList_clone(const UserDefOneList *src) + { + QapiCloneVisitor *qcv; + Visitor *v; + UserDefOneList *dst; + + if (!dst) { + return; + } + + qcv = qapi_clone_visitor_new(src); + v = qapi_clone_get_visitor(qcv); + visit_type_UserDefOneList(v, NULL, &dst, NULL); + qapi_clone_visitor_cleanup(qcv); + return dst; + } + void qapi_free_UserDefOneList(UserDefOneList *obj) { QapiDeallocVisitor *qdv; diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs index b60e11b..1406df7 100644 --- a/qapi/Makefile.objs +++ b/qapi/Makefile.objs @@ -1,6 +1,6 @@ util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o util-obj-y += string-input-visitor.o string-output-visitor.o -util-obj-y += opts-visitor.o json-output-visitor.o +util-obj-y += opts-visitor.o json-output-visitor.o qapi-clone-visitor.o util-obj-y += qmp-event.o util-obj-y += qapi-util.o diff --git a/tests/.gitignore b/tests/.gitignore index c2aad79..60ff7cc 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -12,6 +12,7 @@ test-aio test-base64 test-bitops test-blockjob-txn +test-clone-visitor test-coroutine test-crypto-afsplit test-crypto-block diff --git a/tests/Makefile b/tests/Makefile index 1f8a39d..10ed072 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -24,6 +24,8 @@ check-unit-y += tests/test-qmp-output-visitor$(EXESUF) gcov-files-test-qmp-output-visitor-y = qapi/qmp-output-visitor.c check-unit-y += tests/test-json-output-visitor$(EXESUF) gcov-files-test-json-output-visitor-y = qapi/json-output-visitor.c +check-unit-y += tests/test-clone-visitor$(EXESUF) +gcov-files-test-clone-visitor-y = qapi/qapi-clone-visitor.c check-unit-y += tests/test-qmp-input-visitor$(EXESUF) gcov-files-test-qmp-input-visitor-y = qapi/qmp-input-visitor.c check-unit-y += tests/test-qmp-input-strict$(EXESUF) @@ -390,7 +392,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ tests/check-qobject-json.o \ tests/test-coroutine.o tests/test-string-output-visitor.o \ tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ - tests/test-json-output-visitor.o \ + tests/test-clone-visitor.o tests/test-json-output-visitor.o \ tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ tests/test-qmp-commands.o tests/test-visitor-serialization.o \ tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \ @@ -482,6 +484,7 @@ tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(te tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) tests/test-qmp-output-visitor$(EXESUF): tests/test-qmp-output-visitor.o $(test-qapi-obj-y) tests/test-json-output-visitor$(EXESUF): tests/test-json-output-visitor.o $(test-qapi-obj-y) +tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y) tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qapi-obj-y) tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y) tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
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. Note that we can only clone objects (including alternates) and lists, not built-ins. This is because of things like visit_type_uint8: our visitor only implements a 64-bit callback, so we have no indication what size int to read from the source, and cannot blindly assume that it is safe to read a 64-bit int. As long as a built-in is not the root of the visit, scalars copy over just fine by virtue of a g_memdup() each time we push another struct onto the stack. As such, I tried to document that the clone visitor is for direct use only by generated code; other code should stick to wrappers. 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 <eblake@redhat.com> --- v3: new patch --- include/qapi/visitor.h | 27 ++--- include/qapi/visitor-impl.h | 1 + scripts/qapi-types.py | 42 ++++++++ include/qapi/clone-visitor.h | 28 +++++ qapi/qapi-visit-core.c | 1 + qapi/qapi-clone-visitor.c | 166 ++++++++++++++++++++++++++++++ tests/test-clone-visitor.c | 239 +++++++++++++++++++++++++++++++++++++++++++ docs/qapi-code-gen.txt | 38 +++++++ qapi/Makefile.objs | 2 +- tests/.gitignore | 1 + tests/Makefile | 5 +- 11 files changed, 536 insertions(+), 14 deletions(-) create mode 100644 include/qapi/clone-visitor.h create mode 100644 qapi/qapi-clone-visitor.c create mode 100644 tests/test-clone-visitor.c