Re: [Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions

[Eric Blake <eblake@redhat.com>]

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

On 01/22/2016 05:18 AM, Markus Armbruster wrote:

> Please think twice before from growing the QAPI patch queue further.  We
> really, really need to clear it.  A TODO comment would be welcome,
> though.

Yes, especially with 2.6 soft freeze fast approaching.


>>>> +/**
>>>> + * Prepare to visit an implicit struct.
>>>> + * Similar to visit_start_struct(), except that an implicit struct
>>>> + * represents a subset of keys that are present at the same nesting level
>>>> + * of a common object but as a separate qapi C struct, rather than a new
>>>> + * object at a deeper nesting level.
>>>
>>> I'm afraid this is impenetrable.
>>>
>>> I tried to refresh my memory on visit_start_implicit_struct()'s purpose
>>> by reading code, but that's pretty impenetrable, too.
>>

>>
>> Suggestions on how to better word it are welcome.  I'm basically trying
>> to describe that this function marks the start of a new C struct used as
>> a sub-object while still conceptually parsing the same top-level QAPI
>> struct.
> 
> Thinking aloud...
> 
> QAPI defines both C data types and a QMP wire format.
> 
> The work of mapping between the two is split between generated visitor
> functions and the QMP visitors.  Roughly, the visitor functions direct
> the mapping of the tree structure, and the QMP visitors take care of the
> leaves.
> 
> The QAPI tree structure and the QMP tree structure are roughly the same.
> Exception: some structs are inlined into a parent struct in QMP, but
> stored as a separate, boxed struct in QAPI.  We call them "implicit"
> structs.  We got rid of one case (base structs), but we still got two
> (flat union and alternate members).  I dislike the exception, but we
> need to document what we've got.
> 
> It's mostly handled by the visitor functions, but two visitors need to
> hook into their handling, because they allocate / free the boxed struct:
> the QMP input visitor and the dealloc visitor.
> 
> The QMP output visitor doesn't.  The effect is that the members of the
> implicit struct get inlined into the explicit struct that surrounds it.
> 
> I oversimplified when I said "and a QMP wire format": since we acquired
> string and QemuOpts visitors, we also have a string and QemuOpts format.
> Both are partial: they don't support all of QAPI.
> 
> However, these formats aren't independent of the QMP wire format.  Since
> we use the same visitor functions, and the visitor functions map the
> tree structure, the tree structure gets mapped just like for QMP.
> Almost theoretical, because these visitors don't support most of the
> structure.
> 
> If we wanted a format that doesn't inline implicit structs, the visitor
> would want to implement visit_start_implicit_struct() and
> visit_end_implicit_struct() just like visit_start_struct() and
> visit_end_struct().  Differences:
> 
> * start_implicit_struct() lacks the @name parameter.
> 
> * end_implicit_struct() lacks the @errp now.  Later in this series, this
>   becomes: there is no check_implicit_struct().
> 
> Not inlining implicit structs seems impossible with the current API.
> I'm *not* asking for a change that makes it possible.  Instead, my point
> is: the inlining of implicit structs is baked into the visitor
> interfaces.
> 
> I'm afraid I can't turn this into a reasonable function comment without
> first amending the introduction comment to cover tree structure mapping.
> Ugh.
> 
> Crazy thought: unboxing the implicit struct should make this interface
> wart go away.  If we commit to do that later, we can "solve" our
> documentation problem the same way as for visit_start_union(): FIXME
> should not be needed.

I _want_ to get rid of the boxing.  But as it is not in my current queue
of pending patches, it will have to wait until the current queue is
flushed; so I'm going for documenting it with FIXMEs for now.

Basically, our current flat union representation is:

struct Union {
    Type tag;
    union {
        Subtype1 *one;
        Subtype2 *two;
    } u;
};

which requires two malloc's to completely populate the struct, and where
we access union->u.one->member, or pass union->u.one to a function
taking Subtype1*.  But it _should_ be:

struct Union {
    Type tag;
    union {
        Subtype1 one;
        Subtype2 two;
    } u;
};

where a single malloc is sufficient, and where we access
union->u.one.member, or pass &union->u.one to a function taking Subtype1*.

It's a tree-wide conversion; and may be easier if done in stages (fix
the generator to take a temporary boolean flag on whether a particular
structure uses inline or boxing, then a series of patches adding that
flag to a few QMP commands at a time, then a final patch to clear out
the temporary flag support) rather than all at once.  I'm not sure how
much Coccinelle will help, because I specifically haven't started the
conversion work until after we can get the current backlog flushed.

-- 
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 --]