Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor

[Avi Kivity]

On 06/06/2012 12:11 AM, Michael Roth wrote:
>> 
>> Is is possible to let the compiler process the .c file, with the IDL
>> delimited by some marker?  I like how device models are self contained
>> in one file now.
> 
> It's possible, but only if we inject the generated visitor code into the
> devices via an #include "qapi-generated/<device>-qapi-visit.c";
> 
> I'm not sure how acceptable that is... but it does reduce the churn
> quite a bit.

We could make qc add this #include (or even inject the code directly) by
emitting a new C file (with #line directives to direct the debugger to
the original) and compiling this intermediate file instead of the source.

>> > +There are three cases where state can be suppressed: when it is 
>> > **immutable**,
>> > +**derived**, or **broken**.  
>> 
>> There is a fourth class, non-guest-visible state (below).  There is a
>> fifth class, migrated by other means, which includes memory and block
>> device state, but of course it isn't interesting in this context.
> 
> There's a higher-level annotation, qc_declaration, which denotes what
> devices/structs should be processed by the QIDL compiler (and follow
> it's rules). So there's an implied "handled by other means" for
> everything that falls outside this category.

Right, but within a qc_declaration struct there can be "other means" fields.

>> 
>> <snip>
>> 
>> Suggestion: add a _guest marker for ordinary state.  Fail the build on
>> unmarked fields.  This ensures that some thought is given to each field,
>> instead of having a default that may be correct most of the time, but
>> not always.
> 
> Hmm, I my general thought was that is doesn't hurt to send extra, which
> made serialization a reasonable default course of action.
> 
> But there is indeed a risk of overwriting target state with garbage if
> we don't verify what fields really should/shouldn't be sent. A marker to
> track this does seem useful in that regard...

I don't think the default is unsafe.  I just dislike ABIs being cast
into stone by carelessness, it can be hard to fix up later.

Suppose we have state X and derived state Y that is sent by mistake.
But it can also be said that Y is the state and X derives from it, so
can we ever remove one or the other?  It would be a bigger problem if
there were multiple implementations of the protocol (instead of just
qemu), but still, I'd rather see more thought going into the protcol
when defining it rather than when trying to change it.

> 
>> 
>> Suggestion: add a mandatory position hint (_guest(7) or _pos(7)) that
>> generates ordering within the fields.  This decouples the order of lines
>> in the struct from the protocol, so you can add state where it make
>> sense, or rearrange lines when they don't, and detect copy/paste errors.
>> 
> 
> I'm in agreement with Gerd that the wire protocol we use should support
> field names. I think device state constitutes a small enough percentage
> of total migrated state that the performance impact would be negligable,
> and migration will invariably add some negotiation/compatibility
> functionality on top of the serialization that would make having field
> names intact useful for analyzing/debugging things.
> 
> I personally like the idea of using compressed json, but I think we
> could implement a QObject<->BER mechanism that would provide this as
> well.

I'd like to see BER too.  But we will have to support the old protocol
for quite some time (I'd say at least 3 years from the first release
that supports the new protocol).

We could put the ordering some other place, but that makes it harder to
write qc_declarations.

>> Surely there are available lexer/parser packages?
> 
> This seems promising:
> 
> http://pygments.org/docs/lexerdevelopment/

IMO some external tool is really needed.  I'm sure qc will pick up new
features quickly, so separating the protocol description's description
from the protocol description's parser is important.  You can't get a
lot more meta than that.

-- 
error compiling committee.c: too many arguments to function