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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/02/2016 11:54 AM, Markus Armbruster wrote:
>
>> 
>> qapi-types.h grows by 492 lines (19KiB, +19%).  Roughly one non-blank
>> line per non-simple type, including list types.  It's included all over
>> the place.
>> 
>> qapi-types.c grows by 4212 lines (92KiB, +90%).
>> 
>> Is it a good idea to generate all clone functions?  As far as I can see,
>> we use only a fraction of them.
>
> Would it be worth creating a separate .h file, and only include that
> file in the few places that actually want a clone, rather than making
> every file pay the price of a larger .h?

Possibly, but it might be a bother, as the generator scripts and
Makefiles are designed for one .h + one .c.

>> A sufficiently smart link-time optimizer can get rid of the ones we
>> don't use.  But I consider the "sufficiently smart optimizer" argument a
>> cop out until proven otherwise.
>
> We could also teach the qapi generator to mark specific types as
> cloneable (and then the transitive closure of all subtypes included by
> those types are also cloneable), and only generate the clone functions
> where it matters, rather than for every type.  I can play with that idea
> (basically, adding a 'clone':true flag in the .json file, then figuring
> out how to propagate that through all subtypes).

Related: generation of list types.  We got away with DummyForceArrays
there.

I don't think you need the transitive closure: the generated clone
function doesn't recurse into generated clone functions, it sets up
recursion with the clone visitor.

Taking a step back: the generated clone functions feel like a case of
generitis.  Here's their template:

    %(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src)
    {
        QapiCloneVisitor *qcv;
        Visitor *v;
        %(c_name)s *dst;

        if (!src) {
            return NULL;
        }

        qcv = qapi_clone_visitor_new(src);
        v = qapi_clone_get_visitor(qcv);
        visit_type_%(c_name)s(v, NULL, &dst, &error_abort);
        qapi_clone_visitor_cleanup(qcv);
        return dst;
    }

The job could be done by a single function:

    void *qapi_clone(const void *src,
                     void (*visit_type)(Visitor, const char *, void **,
                                        Error **))
    {
        QapiCloneVisitor *qcv;
        Visitor *v;
        void *dst;

        if (!src) {
            return NULL;
        }

        qcv = qapi_clone_visitor_new(src);
        v = qapi_clone_get_visitor(qcv);
        visit_type(v, NULL, &dst, &error_abort);
        qapi_clone_visitor_cleanup(qcv);
        return dst;
    }

However, passing a visit_type_FOO to this function requires a type cast.
Type-punning functions is always ugly.

A macro would do the job without the type punning.  Inline:

#define QAPI_CLONE(type, src)                                           \
    (src ? ({                                                           \
               QapiCloneVisitor *qcv;                                   \
               Visitor *v;                                              \
               type *dst;                                               \
                                                                        \
               qcv = qapi_clone_visitor_new(src);                       \
               v = qapi_clone_get_visitor(qcv);                         \
               visit_type_ ## type(v, NULL, &dst, &error_abort);        \
               qapi_clone_visitor_cleanup(qcv);                         \
               dst;                                                     \
           })                                                           \
        : NULL)

Out of line:

#define QAPI_DEFINE_CLONE(type, src)                            \
    type *qapi_ ## type ## _clone(const type *src)              \
    {                                                           \
        QapiCloneVisitor *qcv;                                  \
        Visitor *v;                                             \
        type *dst;                                              \
                                                                \
        if (!src) {                                             \
            return NULL;                                        \
        }                                                       \
                                                                \
        qcv = qapi_clone_visitor_new(src);                      \
        v = qapi_clone_get_visitor(qcv);                        \
        visit_type_ ## type(v, NULL, &dst, &error_abort);       \
        qapi_clone_visitor_cleanup(qcv);                        \
        return dst;                                             \
    }

where non-generated code uses QAPI_DEFINE_CLONE() to define the ones we
actually need.

Differently ugly.

Note that the qapi_free_FOO() are similar.  They're used more
frequently, which makes the inline macro less attractive.

>> We already generate plenty of code we don't use, but that's not exactly
>> a reason for generating more code we don't use.
>
> I guess it's a trade-off of whether we will find new uses rather than
> the two immediate places that I fix in the next couple of patches - if
> we need more types to be cloneable, how much maintenance effort is it to
> mark those additional types cloneable?

Compared to writing new code that needs a clone, adding one more line to
get the clone function is nothing.  The time needed to realize you need
to do it and to figure out how to do it may not be nothing.
Documentation problem, I'd say.

>>> +++ b/include/qapi/visitor-impl.h
>>> @@ -35,6 +35,7 @@ typedef enum VisitorType {
>>>      VISITOR_INPUT,
>>>      VISITOR_OUTPUT,
>>>      VISITOR_DEALLOC,
>>> +    VISITOR_CLONE,
>> 
>> It's *two* visitors, running in lockstep: an input visitor visiting
>> @src, and an output visitor visiting the clone.
>> 
>> To which one does the Visitor's type apply?
>> 
>> Let's review how it's used:
>> 
>> * visit_start_struct()
>> 
>>     if (obj && v->type == VISITOR_INPUT) {
>>         assert(!err != !*obj);
>>     }
>> 
>>   The clone visitor behaves like an input visitor here.
>
> It doesn't actually set err, but is indeed allocating *obj.

If it could fail, it would set err then.

Followup question: should we assert this for a clone visitor, too?

>> * visit_type_enum()
>> 
>>     if (v->type == VISITOR_INPUT) {
>>         input_type_enum(v, name, obj, strings, errp);
>>     } else if (v->type == VISITOR_OUTPUT) {
>>         output_type_enum(v, name, obj, strings, errp);
>>     }
>> 
>>   The clone visitor wants to override this completely.
>
> The override is to have visit_type_enum() be a no-op (because we don't
> visit top-level enums, and an enum which is a member of a struct or list
> is cloned as part of the memdup() of pushing into the struct or list).

If we still had the type_enum() method removed in commit da72ab0, we'd
do enums exactly like other scalar types:

    static void qapi_clone_type_enum(Visitor *v, const char *name, int *obj,
                                     const char *const strings[],
                                     Error **errp)
    {
        QapiCloneVisitor *qcv = to_qcv(v);

        assert(qcv->depth);
        /* Value was already cloned by g_memdup() */
    }

> The dealloc visitor also has visit_type_enum() be a no-op.

The dealloc visitor could be viewed as output visitor with a relaxed
precondition: incompletely constructed input is okay.  Anyway, it's
semantically special enough to require its own clauses in the visitor
documentation, so making its own visitor type is justifiable.

>>   Hypothetical input from / output to binary visitors would probably
>>   want the same.  Perhaps this part of commit da72ab0 wasn't a good
>>   idea.
>
> The clone visitor is cloning C structs, not other representations (that
> is, this is NOT a QObject cloner, and therefore even if we add
> hypothetical binary representation input/output visitors, this wil NOT
> be cloning those binary representations).

Of course.  But I was trying to say something else.  Let me try again.

Consider the five visitors QMP input, string input, JSON input
(hypothetical), binary input (hypothetical), clone.

All five build a tree of QAPI objects guided by some (visitor-specific)
input.  Visitor-specific means the visitor core is oblivious of the kind
of input.

For the QMP input visitor, the input is a QObject.

For the string input visitor, the input is a string in idiosyncratic
syntax.

For the hypothetical JSON input visitor, the input would be an RFC 7159
JSON-text.

For the hypothetical binary input visitor, the input would be a bunch of
bytes.

For the clone visitor, the input is another tree of QAPI objects.

My point is: for the visitor core, there is no difference between these
five.  This separation of concerns is only proper.

Except for one place, where we let a visitor detail bleed into the core:
enums.  The bleeding happened in commit da72ab0.  We did it because it
de-duplicated

>                                            The fact that we now have a
> fourth category of visitor, where dealloc and clone visitors behave the
> same for visit_type_enum, didn't seem too bad.

Oh, it's certainly not *bad*.  I'm just wondering whether it's
necessary, and whether the existing conditionals on v->type need
updating.  I also hope to gain a better understanding of it in the
process.

>>> +def gen_type_clone_decl(name):
>>> +    return mcgen('''
>>> +
>>> +%(c_name)s *qapi_%(c_name)s_clone(const %(c_name)s *src);
>> 
>> Hmm.  It's qapi_free_FOO(), but qapi_FOO_clone().
>
> and qapi_visit_FOO_members(). I'm fine swapping names to whatever you
> find nicer, but think qapi_clone_FOO() is probably best now that you
> mention it.
>
>
>>> +static void qapi_clone_start_struct(Visitor *v, const char *name, void 
>>> **obj,
>>> +                                    size_t size, Error **errp)
>>> +{
>>> +    QapiCloneVisitor *qcv = to_qcv(v);
>>> +
>>> +    if (!obj) {
>>> +        /* Nothing to allocate on the virtual walk during an
>>> +         * alternate, but we still have to push depth.
>> 
>> I don't quite get this comment.  I understand why we don't memdup() ---
>> it's pointless without a place to store the duplicate.  I think I
>> understand why we want to increment qcv->depth unconditionally, but I
>> might be wrong.  What exactly is the meaning of member depth?
>> 
>> What's the relation to alternate?
>> 
>> I see this is temporary.  Should I not worry and move on?
>
> I can still make the comment better.
>
> Basically, when we are doing visit_type_ALTERNATE() and happen to pick
> the object branch of the alternate, we call visit_start_struct(NULL)
> under the hood; the 'if (!obj)' should NOT be reachable for a top-level
> visit (because we state that this visitor is limited and cannot do
> virtual walks), but it IS reachable under the hood when walking an
> alternate.

Right...

> So maybe the comment should read:
>
> if (!obj) {
>   /*
>    * Reachable only when visiting an alternate. Nothing to allocate,

an alternate's object branch

>    * and visit_start_alternate() already copied memory, so we have
>    * nothing further to do except increment depth for proper
>    * bookkeeping during visit_end_struct().
>    */
>
>> 
>>> +         * FIXME: passing obj to visit_end_struct would make this easier */
>>> +        assert(qcv->depth);
>>> +        qcv->depth++;
>>> +        return;
>>> +    }
>>> +
>>> +    *obj = g_memdup(qcv->depth ? *obj : qcv->root, size);
>>> +    qcv->depth++;
>>> +}
>
>>> +static void qapi_clone_type_any(Visitor *v, const char *name, QObject 
>>> **obj,
>>> +                                 Error **errp)
>>> +{
>>> +    QapiCloneVisitor *qcv = to_qcv(v);
>>> +    assert(qcv->depth);
>>> +    /* Pointer was already copied by g_memdup; fix the refcount */
>>> +    qobject_incref(*obj);
>> 
>> 'any' values are shared?
>
> Unless you know of a way to deep clone QObject, and a reason that a deep
> clone is better than sharing.  The reason we have to deep clone the rest
> of the QAPI object is that QAPI doesn't have ref-counting the way
> QObject does.

Simplifying management of the object life times is one reason to clone.
The other, more important one is to avoid unwanted sharing of
modifications.  Only applies for mutable data, of course.

So yes, life time management is one reason for deep cloning QAPI, and it
doesn't apply to the 'any' values.  But the other reason does apply,
since 'any' values can be mutable.

Options:

(1) Document 'any' values are shared between the clone and the original.
    Dangerous trap, if you ask me.

(2) Don't implement type_any().  Attempts to clone something containing
    'any' values crash.  Document that as a restriction.  Ugly: we can
    generate clone functions that cannot be used.  This leads to:

(3) Like (2), but additionally refrain from generating clone functions
    that can crash.  More restrive than (2), because with (2) such a
    clone function still works when the 'any' values are optional and
    absent, including empty ['any'].

(4) Implement QObject cloning.

>>> +    UserDefOne *qapi_UserDefOne_clone(const UserDefOne *src)
>>> +    {
>>> +        QapiCloneVisitor *qcv;
>>> +        Visitor *v;
>>> +   UserDefOne *dst;
>> 
>> Tab damage.  More of the same below.
>
> I can't make emacs figure out that the docs file doesn't need TABs, so
> it's not the first time I've tab-damaged a patch (although sometimes
> I've managed to catch it before leaking it to the list - not this time,
> though).

We can't disable indent-tabs-mode for all files in .dir-locals.el,
because that would wreak havoc with Makefiles.

We could try disabling it for text-mode.

If we're bold, we could try disabling it for everything, then enable it
for makefile-mode.