Re: [Qemu-devel] [PATCH v4 24/26] qidl: add QAPI-based code generator

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, Oct 16, 2012 at 09:20:09AM +0200, Paolo Bonzini wrote:
> Il 15/10/2012 18:35, Michael Roth ha scritto:
> >> - immutable/derived/broken/elsewhere (and the default, let's call it
> >> serialized) are really five cases of the same QIDL property.  Perhaps
> >> this could be enforced in the extended syntax like this:
> >>
> >>     #define q_immutable QIDL(serialize(immutable))
> >>     #define q_derived QIDL(serialize(derived))
> >>     #define q_broken QIDL(serialize(broken))
> >>     #define q_elsewhere QIDL(serialize(elsewhere))
> >>
> >> I would also make it possible to explicitly specify the fifth state, if
> >> only for symmetry.
> > 
> > Agreed, that's a more proper grouping. Though, for consistency with
> > QIDL(property, ...), I would do QIDL(serialize, ...)
> 
> Sure.
> 
> >> - it would be _much_ better if you could automatically derive properties
> >> information for embedded structs.  For example, Notifiers and qemu_irqs
> >> are always q_immutable.  NotifierLists probably are always q_elsewhere,
> >> because the owner of the notifiers should add themselves back.
> > 
> > q_inherit maybe? Otherwise we're overriding "q_default" in subtle
> > ways that may not always be desired.
> 
> I think the default should be "whatever makes more sense", which for
> QIDL_DECLAREd types means making the member q_immutable if it makes
> sense for the type.
> 
> It's too fragile to expect all subclasses to know whether their
> superclass is immutable or has to be serialized, so q_inherit should be
> the default.  For atomic types, q_inherit is the same as q_serialized.

Totally agree. And over time, as more and more device-related structures are
QIDL_DECLAREd, this will help a lot on cutting down "boilerplate"
annotations.

> 
> That said, an alternative is just to never declare the superclass
> q_immutable.  That would work as long as you do not restrict QIDL to
> DeviceState subclasses---see attached patch.

Yah, I think the best general policy is to make as many of the fields
Just Work as possible by default. This can be done by using QIDL_DECLARE()
for as many relevant struct fields as possible (and the ones that aren't
QIDL_DECLAREd (or have an open-coded visitor) will generate errors that
can prompt further action (the optimal/encouraged one being to QIDL_DECLARE
the struct in question, or creating an open-coded visitor for the
simpler cases.

This how I'd prefer we handle making inherit-by-default work better, and
I'll focus on this more in the conversions.

> 
> >> In general, if struct X is QIDL_DECLAREd and only has q_immutable
> >> fields, it can be taken as q_immutable.  Hence for example the base
> >> class should not need any decoration; ISADevice will be seen as
> >> q_immutable, but PCIDevice will be seen as serialized.  But even if a
> >> struct is not QIDL_DECLAREd, it  should be possible to apply a tag to a
> >> typedef, and have it always applied to the members.
> 
> Hmm, this wasn't the best choice of words.  What I actually meant was
> "to apply a tag to a typedef, and have it always applied to members of
> that type in other structs".  Like
> 
> typedef struct Notifier Notifier q_immutable;

This would be really nice, but the parser is gonna need more hardening
before we can remove it's "scan for lines beginning with QIDL_START* and
parse what we expect to come afterward" behavior. There's also
build time implications. An alternative I'm considering is:

typedef struct Notifier Notifier;

QIDL_DECLARE_PUBLIC(Notifier, q_immutable) {
    ...
}

This will make all the fields q_immutable by default (individual fields
can override it in the future when the need arises). Then we teach the
code generator to drop serialization for any struct fields who don't
have any serializeable fields (mainly to avoid a bunch of nil fields in
the serialized code (e.g. 'notifier1': {})).

I'm planning on doing the above in the context of the device conversions
to get them a little more palatable.

Seem reasonable?

> 
> Note that Notifier will never have any serializable state, hence it will
> not be QIDL_DECLAREd.  It is just a proxy that specifies a function to call.
> 
> While in principle it is possible to change the function at run-time,
> that's not the way that Notifiers are used.  That can still be
> documented using q_elsewhere, but I think that sane defaults other than
> q_serialized are useful to avoid cluttering the declarations.  However,
> this is a very minor qualm.
> 
> Paolo