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
signature.asc
Description: OpenPGP digital signature
