| Message ID | 1463632874-28559-14-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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. > > 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!). > > 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. > > Scalars not at the root of the clone copy just fine, by virtue of a > g_memdup() each time we push another struct onto the stack. > > 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. > > 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. > > 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> > > --- > v4: hoist earlier in series, drop wasted generated functions and > replace it with QAPI_CLONE() macro, drop 'any' support, tweak > core assertion checking, internal improve commit message > v3: new patch > --- > include/qapi/visitor.h | 33 ++++--- > include/qapi/visitor-impl.h | 12 +-- > include/qapi/clone-visitor.h | 37 ++++++++ > qapi/qapi-visit-core.c | 18 ++-- > qapi/qapi-clone-visitor.c | 178 +++++++++++++++++++++++++++++++++++++ > tests/test-clone-visitor.c | 207 +++++++++++++++++++++++++++++++++++++++++++ > qapi/Makefile.objs | 2 +- > tests/.gitignore | 1 + > tests/Makefile | 4 + > 9 files changed, 466 insertions(+), 26 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 b3bd97c..3f46921 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. [...] * 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 * visitor safely handles incomplete objects. Since input visitors * never produce an incomplete object, such an object is possible only * by manual construction. What about the clone visitor? > @@ -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); > * > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 16e0b86..29fac2b 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -27,14 +27,16 @@ > */ > > /* > - * 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. We'll see below that the code now wants to accept a VISITOR_CLONE in addition to VISITOR_INPUT in most places, and similarly in addition to VISITOR_OUTPUT. Subtle. > */ > typedef enum VisitorType { > - VISITOR_INPUT, > - VISITOR_OUTPUT, > - VISITOR_DEALLOC, > + VISITOR_INPUT = 1, > + VISITOR_OUTPUT = 2, > + VISITOR_CLONE = 3, > + VISITOR_DEALLOC = 4, > } VisitorType; > > struct Visitor [...] /*** Visiting structures ***/ /* * Start visiting an object @obj (struct or union). * * @name expresses the relationship of this object to its parent * 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. What about the clone visitor? * * @errp obeys typical error usage, and reports failures such as a * member @name is not present, or present but not an object. On * error, input visitors set *@obj to NULL. What about the clone visitor? * * 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 * 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? */ void visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp); [...] /*** Visiting lists ***/ /* * Start visiting a list. * * @name expresses the relationship of this list to its parent * 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 * ignored. What about the clone visitor? * * @errp obeys typical error usage, and reports failures such as a * member @name is not present, or present but not a list. On error, * input visitors set *@list to NULL. What about the clone visitor? * * After visit_start_list() succeeds, the caller may visit its members * one after the other. A real visit (where @obj is non-NULL) uses * visit_next_list() for traversing the linked list, while a virtual * 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 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); [...] /*** Visiting alternates ***/ /* * Start the visit of an alternate @obj. * * @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. What about the clone visitor? * * If @promote_int, treat integers as QTYPE_FLOAT. * * 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. */ [...] /*** Other helpers ***/ /* * Does optional struct member @name need visiting? * * @name must not be NULL. This function is only useful between * visit_start_struct() and visit_end_struct(), since only objects * have optional keys. * * @present points to the address of the optional member's has_ flag. * * Input visitors set *@present according to input; other visitors * leave it unchanged. In either case, return *@present for * convenience. I guess this is correct for the clone visitor. */ bool visit_optional(Visitor *v, const char *name, bool *present); /* * Visit an enum value. * * @name expresses the relationship of this enum to its parent * container; see the general description of @name above. * * @obj must be non-NULL. Input visitors parse input and set *@obj to * the enumeration value, leaving @obj unchanged on error; other * visitors use *@obj but leave it unchanged. I guess this is correct for the clone visitor. * * Currently, all input visitors parse text input, and all output * visitors produce text output. The mapping between enumeration * values and strings is done by the visitor core, using @strings; it * should be the ENUM_lookup array from visit-types.h. * * May call visit_type_str() under the hood, and the enum visit may * fail even if the corresponding string visit succeeded; this implies * that visit_type_str() must have no unwelcome side effects. */ void visit_type_enum(Visitor *v, const char *name, int *obj, const char *const strings[], Error **errp); /* * Check if visitor is an input visitor. Does the clone visitor count as input visitor here? Should it? */ bool visit_is_input(Visitor *v); /*** Visiting built-in types ***/ /* * Visit an integer value. * * @name expresses the relationship of this integer to its parent * container; see the general description of @name above. * * @obj must be non-NULL. Input visitors set *@obj to the value; * other visitors will leave *@obj unchanged. I guess this is correct for the clone visitor. */ void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp); [...] /* * Visit a boolean value. * * @name expresses the relationship of this boolean to its parent * container; see the general description of @name above. * * @obj must be non-NULL. Input visitors set *@obj to the value; * other visitors will leave *@obj unchanged. I guess this is correct for the clone visitor. */ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); /* * Visit a string value. * * @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 "". I guess this is correct for the clone visitor. * * It is safe to cast away const when preparing a (const char *) value * into @obj for use by an output visitor. * * FIXME: Callers that try to output NULL *obj should not be allowed. */ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); /* * Visit a number (i.e. double) value. * * @name expresses the relationship of this number to its parent * container; see the general description of @name above. * * @obj must be non-NULL. Input visitors set *@obj to the value; * other visitors will leave *@obj unchanged. Visitors should * document if infinity or NaN are not permitted. I guess this is correct for the clone visitor. */ void visit_type_number(Visitor *v, const char *name, double *obj, Error **errp); /* * Visit an arbitrary value. * * @name expresses the relationship of this value to its parent * container; see the general description of @name above. * * @obj must be non-NULL. Input visitors set *@obj to the value; * other visitors will leave *@obj unchanged. *@obj must be non-NULL * for output visitors. Fine, as the clone visitor doesn't support any. */ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp); > 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)) This casts visit_type_FOO(), where FOO is a QAPI-generated complex or alternate type, from void (*)(Visitor *, const char *, FOO **, Error **) to void (*)(Visitor *, const char *, void **, Error **) Okay. Can't see how to avoid the type-punning without generating lots of wrapper functions. Note that scalar QAPI types have one * less: FOO * instead of FOO **. But they're explicitly not permitted here. > + > +#endif > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 279ea8e..c5bdca2 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c As we'll see further down, @obj points into the clone, except at the root, where it points to qapi_clone()'s local variable @dst. A pointer-valued *@obj still points into the source. Now let's go through the v->type checks real slow. First one: void visit_complete(Visitor *v, void *opaque) { if (v->type == VISITOR_OUTPUT) { assert(v->complete); } An output visitor needs a complete(), because without it, there's no way to get the output (unless you resort to side effects). qapi-visit-core.c chooses to enforce this. Not really necessary, because qapi-visit-core.c works just fine without it. The clone visitor has no use for complete, because it returns its output differently: qapi_clone() returns it. Therefore, we don't want to treat clone as output here. Okay. > @@ -44,10 +44,10 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, > > if (obj) { > assert(size); > - assert(v->type != VISITOR_OUTPUT || *obj); > + assert(!(v->type & VISITOR_OUTPUT) || *obj); > } For real walks (obj != NULL): * Input visitors write *obj, and don't care for the old value. * Output visitors read *obj, and a struct can't be null. * The dealloc visitor reads *obj, but null is fine (partially constructed object). * The clone visitor reads like an output visitor (except at the root) and writes like an input visitor. Before the patch, we assert "if output visitor, then *obj isn't null". After the patch, we do the same for the clone visitor. Correct, except at the root. There, @obj points to qapi_clone()'s @dst, which is uninitialized. I'm afraid this assertion fails if @dst happens to be null. Can we fix this by removing the "except at the root" special case? Change qapi_clone to initialize dst = src, drop QapiCloneVisitor member @root and qapi_clone_visitor_new() parameter @src. > v->start_struct(v, name, obj, size, &err); > - if (obj && v->type == VISITOR_INPUT) { > + if (obj && (v->type & VISITOR_INPUT)) { > assert(!err != !*obj); > } Before the patch, we assert "input visitor must either fail or create *obj for a real walk." After the patch, we do the same for the clone visitor. Okay. > error_propagate(errp, err); > @@ -72,7 +72,7 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list, > > assert(!list || size >= sizeof(GenericList)); > v->start_list(v, name, list, size, &err); > - if (list && v->type == VISITOR_INPUT) { > + if (list && (v->type & VISITOR_INPUT)) { > assert(!(err && *list)); > } Likewise. > error_propagate(errp, err); > @@ -96,11 +96,11 @@ void visit_start_alternate(Visitor *v, const char *name, > Error *err = NULL; > > assert(obj && size >= sizeof(GenericAlternate)); > - assert(v->type != VISITOR_OUTPUT || *obj); > + assert(!(v->type & VISITOR_OUTPUT) || *obj); > if (v->start_alternate) { > v->start_alternate(v, name, obj, size, promote_int, &err); > } > - if (v->type == VISITOR_INPUT) { > + if (v->type & VISITOR_INPUT) { > assert(v->start_alternate && !err != !*obj); > } > error_propagate(errp, err); Same analysis as for visit_start_struct(). [...] bool visit_is_input(Visitor *v) { return v->type == VISITOR_INPUT; } This answers my question "Does the clone visitor count as input visitor here?" Remaining question: "Should it?" > @@ -252,9 +252,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) > assert(obj); > /* TODO: Fix callers to not pass NULL when they mean "", so that we > * can enable: > - assert(v->type != VISITOR_OUTPUT || *obj); > + assert(!(v->type & VISITOR_OUTPUT) || *obj); > */ > v->type_str(v, name, obj, &err); > + /* Likewise, use of NULL means we can't do (v->type & VISITOR_INPUT) */ > if (v->type == VISITOR_INPUT) { > assert(!err != !*obj); > } If your head doesn't hurt by know, you either wrote this, or you're not reading closely :) If the TODOs were already addressed, we'd again get the same analysis as for visit_start_struct(), except for the arguments about the root, which don't apply here, because the clone visitor doesn't accept scalar roots. In the current state, the analysis needs to be modified as follows. First assertion: Before the patch, we'd like to assert "if output or clone visitor, then *obj isn't null". We can't as long as we need to treat null as the empty string. After the patch, the situation is the same for the clone visitor. Okay. Second assertion: Before the patch, we assert "input visitor must either fail or create *obj for a real walk." The TODO doesn't apply; we create "", not null. After the patch, we'd like to assert the same for the clone visitor, but we can't: the clone of null is null. Okay. > @@ -273,9 +274,9 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) > Error *err = NULL; > > assert(obj); > - assert(v->type != VISITOR_OUTPUT || *obj); > + assert(!(v->type & VISITOR_OUTPUT) || *obj); > v->type_any(v, name, obj, &err); > - if (v->type == VISITOR_INPUT) { > + if (v->type & VISITOR_INPUT) { > assert(!err != !*obj); > } > error_propagate(errp, err); v->type_any() will crash for the clone visitor, so these changes aren't. necessary. If you want them to future-proof the code, I need to convince myself the changes make sense, similar to what I did for the other ones in this file. > @@ -342,4 +343,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 */ > } I'm upgrade my verdict from "subtle" to "scarily subtle" %-} > diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c > new file mode 100644 > index 0000000..a24a258 > --- /dev/null > +++ b/qapi/qapi-clone-visitor.c > @@ -0,0 +1,178 @@ > +/* > + * Copy one QAPI object to another > + * > + * 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 "qapi/clone-visitor.h" > +#include "qapi/visitor-impl.h" > +#include "qapi/error.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) { > + /* Only possible when visiting an alternate's object > + * branch. Nothing to do here, since the earlier > + * visit_start_alternate() already copied memory. */ Should visitor-impl.h explain how method start_struct() is used with alternates? I once again forgot how this works... Hmm, you explained it to me during review of v3. Despite there's "nothing to do here", you found something to do: > + assert(qcv->depth); > + return; > + } > + > + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); > + qcv->depth++; > +} > + > +static void qapi_clone_end(Visitor *v, void **obj) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + > + assert(qcv->depth); > + if (obj) { > + 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_null(Visitor *v, const char *name, Error **errp) > +{ > + QapiCloneVisitor *qcv = to_qcv(v); > + > + assert(qcv->depth); > + /* Nothing to do */ > +} > + > +static void qapi_clone_free(Visitor *v) > +{ > + g_free(v); > +} > + > +static Visitor *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_null = qapi_clone_type_null; > + v->visitor.free = qapi_clone_free; > + > + return &v->visitor; > +} > + > +void *qapi_clone(const void *src, void (*visit_type)(Visitor *, const char *, > + void **, Error **)) > +{ > + Visitor *v; > + void *dst; > + > + if (!src) { > + return NULL; > + } > + > + v = qapi_clone_visitor_new(src); > + visit_type(v, NULL, &dst, &error_abort); > + visit_free(v); > + return dst; > +} [Skipping the tests for now to get this review out today...]
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: [...] >> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c >> index 279ea8e..c5bdca2 100644 >> --- a/qapi/qapi-visit-core.c >> +++ b/qapi/qapi-visit-core.c [...] >> @@ -44,10 +44,10 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, >> >> if (obj) { >> assert(size); >> - assert(v->type != VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> } > > For real walks (obj != NULL): > > * Input visitors write *obj, and don't care for the old value. > > * Output visitors read *obj, and a struct can't be null. > > * The dealloc visitor reads *obj, but null is fine (partially > constructed object). > > * The clone visitor reads like an output visitor (except at the root) > and writes like an input visitor. > > Before the patch, we assert "if output visitor, then *obj isn't null". > > After the patch, we do the same for the clone visitor. Correct, except > at the root. There, @obj points to qapi_clone()'s @dst, which is > uninitialized. I'm afraid this assertion fails if @dst happens to be > null. I can observe this failure in make check when I compile with optimization. [...]
On 06/02/2016 07:43 AM, Markus Armbruster wrote: > 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. >> > [...] > * 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 > * visitor safely handles incomplete objects. Since input visitors > * never produce an incomplete object, such an object is possible only > * by manual construction. > > What about the clone visitor? Probably safest to document it as undefined on incomplete objects. > /* > * Start visiting an object @obj (struct or union). > * > * @name expresses the relationship of this object to its parent > * 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. > > What about the clone visitor? Yes, clone visitors also use size. > > * > * @errp obeys typical error usage, and reports failures such as a > * member @name is not present, or present but not an object. On > * error, input visitors set *@obj to NULL. > > What about the clone visitor? Never sets an error (ie. it can't fail on a complete source object, if you don't include abort-due-to-OOM scenarios), so I'm not sure I need to word anything differently here. > * Start visiting a list. > * > * @name expresses the relationship of this list to its parent > * 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 > * ignored. > > What about the clone visitor? > > * > * @errp obeys typical error usage, and reports failures such as a > * member @name is not present, or present but not a list. On error, > * input visitors set *@list to NULL. > > What about the clone visitor? Same as for start_struct. > /* > * Does optional struct member @name need visiting? > * > * @name must not be NULL. This function is only useful between > * visit_start_struct() and visit_end_struct(), since only objects > * have optional keys. > * > * @present points to the address of the optional member's has_ flag. > * > * Input visitors set *@present according to input; other visitors > * leave it unchanged. In either case, return *@present for > * convenience. > > I guess this is correct for the clone visitor. Clone visitor leaves it alone (it is reading *@present on the dest, which was already set earlier during the g_memdup() of visit_start_*). > /* > * Visit an enum value. > * > * @name expresses the relationship of this enum to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors parse input and set *@obj to > * the enumeration value, leaving @obj unchanged on error; other > * visitors use *@obj but leave it unchanged. > > I guess this is correct for the clone visitor. It's a bit of a stretch, but "use *@obj" can certainly mean "do nothing with it, because it is a scalar that was already set earlier during the g_memdup() of visit_start_*". So yes, the clone visitor wants visit_type_enum() to be a no-op. > > /* > * Check if visitor is an input visitor. > > Does the clone visitor count as input visitor here? Should it? No, and probably no. A clone visitor never sets errp, and therefore there is no reason to clean up after a failed clone; and our current use of visit_is_input() is only for cleaning up after failures in an input visitor. > > */ > bool visit_is_input(Visitor *v); > > /*** Visiting built-in types ***/ > > /* > * Visit an integer value. > * > * @name expresses the relationship of this integer to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors set *@obj to the value; > * other visitors will leave *@obj unchanged. > > I guess this is correct for the clone visitor. Again correct - the clone visitor doesn't set anything at this point, because the integer was already copied earlier during the g_memdup() of visit_start_*(). > /* > * Visit a string value. > * > * @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 "". > > I guess this is correct for the clone visitor. The clone visitor could morph NULL into "" (I didn't code it that way, though). Here, the clone visitor DOES set *@obj, in order to dedupe the pointer from the source object, so maybe a third sentence is needed? > /* > * Visit an arbitrary value. > * > * @name expresses the relationship of this value to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors set *@obj to the value; > * other visitors will leave *@obj unchanged. *@obj must be non-NULL > * for output visitors. > > Fine, as the clone visitor doesn't support any. It could, if we use the JSON output visitor code later in the series to create a QObject deep-cloner, but I'd rather not do it unless we find an actual need (keeping 'any' out of clone does simplify the number of corner cases to think about). >> +++ b/qapi/qapi-visit-core.c > > As we'll see further down, @obj points into the clone, except at the > root, where it points to qapi_clone()'s local variable @dst. A > pointer-valued *@obj still points into the source. > > Now let's go through the v->type checks real slow. > >> @@ -44,10 +44,10 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, >> >> if (obj) { >> assert(size); >> - assert(v->type != VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> } > > For real walks (obj != NULL): > > * Input visitors write *obj, and don't care for the old value. > > * Output visitors read *obj, and a struct can't be null. > > * The dealloc visitor reads *obj, but null is fine (partially > constructed object). > > * The clone visitor reads like an output visitor (except at the root) > and writes like an input visitor. > > Before the patch, we assert "if output visitor, then *obj isn't null". > > After the patch, we do the same for the clone visitor. Correct, except > at the root. There, @obj points to qapi_clone()'s @dst, which is > uninitialized. I'm afraid this assertion fails if @dst happens to be > null. > > Can we fix this by removing the "except at the root" special case? > Change qapi_clone to initialize dst = src, drop QapiCloneVisitor member > @root and qapi_clone_visitor_new() parameter @src. Cool idea! And it avoids the crash (I was indeed compiling without optimization, and getting lucky that the uninit value wasn't crashing my tests; wonder why valgrind wasn't flagging it). > [...] > bool visit_is_input(Visitor *v) > { > return v->type == VISITOR_INPUT; > } > > This answers my question "Does the clone visitor count as input visitor > here?" Remaining question: "Should it?" I'm still not convinced, again on the grounds that this is used for cleanup after a failed visit, but clone visits don't fail. > >> @@ -252,9 +252,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) >> assert(obj); >> /* TODO: Fix callers to not pass NULL when they mean "", so that we >> * can enable: >> - assert(v->type != VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> */ >> v->type_str(v, name, obj, &err); >> + /* Likewise, use of NULL means we can't do (v->type & VISITOR_INPUT) */ >> if (v->type == VISITOR_INPUT) { >> assert(!err != !*obj); >> } > > If your head doesn't hurt by know, you either wrote this, or you're not > reading closely :) And there's my idea of making the clone visitor auto-magically clone NULL into "", at which point the conditions in the assertions would change. > > If the TODOs were already addressed, we'd again get the same analysis as > for visit_start_struct(), except for the arguments about the root, which > don't apply here, because the clone visitor doesn't accept scalar roots. > > In the current state, the analysis needs to be modified as follows. > > First assertion: > > Before the patch, we'd like to assert "if output or clone visitor, then > *obj isn't null". We can't as long as we need to treat null as the > empty string. > > After the patch, the situation is the same for the clone visitor. Okay. > > Second assertion: > > Before the patch, we assert "input visitor must either fail or create > *obj for a real walk." The TODO doesn't apply; we create "", not null. > > After the patch, we'd like to assert the same for the clone visitor, but > we can't: the clone of null is null. Okay. > >> @@ -273,9 +274,9 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) >> Error *err = NULL; >> >> assert(obj); >> - assert(v->type != VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> v->type_any(v, name, obj, &err); >> - if (v->type == VISITOR_INPUT) { >> + if (v->type & VISITOR_INPUT) { >> assert(!err != !*obj); >> } >> error_propagate(errp, err); > > v->type_any() will crash for the clone visitor, so these changes aren't. > necessary. If you want them to future-proof the code, I need to > convince myself the changes make sense, similar to what I did for the > other ones in this file. Okay, I can leave this hunk out for now. > >> @@ -342,4 +343,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 */ >> } > > I'm upgrade my verdict from "subtle" to "scarily subtle" %-} > Any comments I can add to make it more obvious to the next reader? >> + >> +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) { >> + /* Only possible when visiting an alternate's object >> + * branch. Nothing to do here, since the earlier >> + * visit_start_alternate() already copied memory. */ > > Should visitor-impl.h explain how method start_struct() is used with > alternates? I once again forgot how this works... Hmm, you explained > it to me during review of v3. > > Despite there's "nothing to do here", you found something to do: > >> + assert(qcv->depth); That's really only asserting that the clone itself is a real visit; we don't allow cloning on a virtual visit, even though the real visit of an alternate also involves the virtual visit of an object, if the alternate's object branch is selected. > [Skipping the tests for now to get this review out today...] Did you want to review the tests in any further detail?
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index b3bd97c..3f46921 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. @@ -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); * diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 16e0b86..29fac2b 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -27,14 +27,16 @@ */ /* - * 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. */ 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/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/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 279ea8e..c5bdca2 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -44,10 +44,10 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, if (obj) { assert(size); - assert(v->type != VISITOR_OUTPUT || *obj); + assert(!(v->type & VISITOR_OUTPUT) || *obj); } v->start_struct(v, name, obj, size, &err); - if (obj && v->type == VISITOR_INPUT) { + if (obj && (v->type & VISITOR_INPUT)) { assert(!err != !*obj); } error_propagate(errp, err); @@ -72,7 +72,7 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list, assert(!list || size >= sizeof(GenericList)); v->start_list(v, name, list, size, &err); - if (list && v->type == VISITOR_INPUT) { + if (list && (v->type & VISITOR_INPUT)) { assert(!(err && *list)); } error_propagate(errp, err); @@ -96,11 +96,11 @@ void visit_start_alternate(Visitor *v, const char *name, Error *err = NULL; assert(obj && size >= sizeof(GenericAlternate)); - assert(v->type != VISITOR_OUTPUT || *obj); + assert(!(v->type & VISITOR_OUTPUT) || *obj); if (v->start_alternate) { v->start_alternate(v, name, obj, size, promote_int, &err); } - if (v->type == VISITOR_INPUT) { + if (v->type & VISITOR_INPUT) { assert(v->start_alternate && !err != !*obj); } error_propagate(errp, err); @@ -252,9 +252,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) assert(obj); /* TODO: Fix callers to not pass NULL when they mean "", so that we * can enable: - assert(v->type != VISITOR_OUTPUT || *obj); + assert(!(v->type & VISITOR_OUTPUT) || *obj); */ v->type_str(v, name, obj, &err); + /* Likewise, use of NULL means we can't do (v->type & VISITOR_INPUT) */ if (v->type == VISITOR_INPUT) { assert(!err != !*obj); } @@ -273,9 +274,9 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) Error *err = NULL; assert(obj); - assert(v->type != VISITOR_OUTPUT || *obj); + assert(!(v->type & VISITOR_OUTPUT) || *obj); v->type_any(v, name, obj, &err); - if (v->type == VISITOR_INPUT) { + if (v->type & VISITOR_INPUT) { assert(!err != !*obj); } error_propagate(errp, err); @@ -342,4 +343,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..a24a258 --- /dev/null +++ b/qapi/qapi-clone-visitor.c @@ -0,0 +1,178 @@ +/* + * Copy one QAPI object to another + * + * 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 "qapi/clone-visitor.h" +#include "qapi/visitor-impl.h" +#include "qapi/error.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) { + /* Only possible when visiting an alternate's object + * branch. Nothing to do here, since the earlier + * visit_start_alternate() already copied memory. */ + assert(qcv->depth); + return; + } + + *obj = g_memdup(qcv->depth ? *obj : qcv->root, size); + qcv->depth++; +} + +static void qapi_clone_end(Visitor *v, void **obj) +{ + QapiCloneVisitor *qcv = to_qcv(v); + + assert(qcv->depth); + if (obj) { + 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_null(Visitor *v, const char *name, Error **errp) +{ + QapiCloneVisitor *qcv = to_qcv(v); + + assert(qcv->depth); + /* Nothing to do */ +} + +static void qapi_clone_free(Visitor *v) +{ + g_free(v); +} + +static Visitor *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_null = qapi_clone_type_null; + v->visitor.free = qapi_clone_free; + + return &v->visitor; +} + +void *qapi_clone(const void *src, void (*visit_type)(Visitor *, const char *, + void **, Error **)) +{ + Visitor *v; + void *dst; + + if (!src) { + return NULL; + } + + v = qapi_clone_visitor_new(src); + visit_type(v, NULL, &dst, &error_abort); + visit_free(v); + return dst; +} diff --git a/tests/test-clone-visitor.c b/tests/test-clone-visitor.c new file mode 100644 index 0000000..ab3fead --- /dev/null +++ b/tests/test-clone-visitor.c @@ -0,0 +1,207 @@ +/* + * 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_CLONE(UserDefOne, 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_CLONE(AltStrBool, 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_CLONE(AltStrBool, 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_CLONE(uint8List, 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_CLONE(Empty2, src); + g_assert(dst); + qapi_free_Empty2(src); + qapi_free_Empty2(dst); +} + +static void test_clone_complex1(void) +{ + UserDefNativeListUnion *src, *dst; + + src = g_new0(UserDefNativeListUnion, 1); + src->type = USER_DEF_NATIVE_LIST_UNION_KIND_STRING; + + dst = QAPI_CLONE(UserDefNativeListUnion, src); + g_assert(dst); + g_assert_cmpint(dst->type, ==, src->type); + g_assert(!dst->u.string.data); + + 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; + /* FIXME: As long as QMP output visitor allows NULL in place of "" + * for strings, the clone visitor must do likewise. */ + src->alt->u.udfu.string = NULL; + 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_CLONE(WrapAlternate, 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, ==, NULL); + 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_CLONE(__org_qemu_x_Struct2, 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/qapi/Makefile.objs b/qapi/Makefile.objs index 2278970..7ea4aeb 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 +util-obj-y += opts-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 2f8c7ea..346d75d 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 1c5cb34..d596493 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -22,6 +22,8 @@ check-unit-y += tests/check-qobject-json$(EXESUF) gcov-files-check-qobject-json-y = qobject/qobject-json.c 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-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) @@ -388,6 +390,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-clone-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 \ @@ -478,6 +481,7 @@ tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $( tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y) 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-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. 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!). 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. Scalars not at the root of the clone copy just fine, by virtue of a g_memdup() each time we push another struct onto the stack. 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. 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. 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> --- v4: hoist earlier in series, drop wasted generated functions and replace it with QAPI_CLONE() macro, drop 'any' support, tweak core assertion checking, internal improve commit message v3: new patch --- include/qapi/visitor.h | 33 ++++--- include/qapi/visitor-impl.h | 12 +-- include/qapi/clone-visitor.h | 37 ++++++++ qapi/qapi-visit-core.c | 18 ++-- qapi/qapi-clone-visitor.c | 178 +++++++++++++++++++++++++++++++++++++ tests/test-clone-visitor.c | 207 +++++++++++++++++++++++++++++++++++++++++++ qapi/Makefile.objs | 2 +- tests/.gitignore | 1 + tests/Makefile | 4 + 9 files changed, 466 insertions(+), 26 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