On 06/02/2016 07:43 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
