Re: [Qemu-devel] [PATCH v4 13/28] qapi: Add new clone visitor

[Eric Blake]

On 06/02/2016 07:43 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 address@hidden 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
>     * address@hidden  @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 address@hidden 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
>     * address@hidden (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 address@hidden 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 address@hidden according to input; other visitors
>     * leave it unchanged.  In either case, return address@hidden for
>     * convenience.
> 
> I guess this is correct for the clone visitor.

Clone visitor leaves it alone (it is reading address@hidden 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 
> address@hidden to
>     * the enumeration value, leaving @obj unchanged on error; other
>     * visitors use address@hidden but leave it unchanged.
> 
> I guess this is correct for the clone visitor.

It's a bit of a stretch, but "use address@hidden" 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 address@hidden to the value;
>     * other visitors will leave address@hidden 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 address@hidden to the value
>     * (never NULL).  Other visitors leave address@hidden 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 address@hidden, 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 address@hidden to the value;
>     * other visitors will leave address@hidden unchanged.  address@hidden 
> 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 address@hidden 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