Re: [Qemu-devel] [PATCH v3 15/18] qapi: Add new clone visitor

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6575 bytes --]

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    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]