Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions

[Kevin Wolf]

Am 11.07.2013 um 21:47 hat Eric Blake geschrieben:
> On 07/09/2013 03:53 AM, Kevin Wolf wrote:
> > The discriminator for anonymous unions is the data type. This allows to
> > have a union type that allows both of these:
> > 
> >     { 'file': 'my_existing_block_device_id' }
> >     { 'file': { 'filename': '/tmp/mydisk.qcow2', 'read-only': true } }
> > 
> > Unions like this are specified in the schema with an empty dict as
> > discriminator. For this example you could take:
> > 
> >     { 'union': 'BlockRef',
> >       'discriminator': {},
> >       'data': { 'definition': 'BlockOptions'
> >                 'reference': 'str' } }
> >     { 'type': 'ExampleObject',
> >       'data: { 'file': 'BlockRef' } }
> 
> Yay - a commit message that shows the new QMP wire format, and the
> qapi-schema.json that generated it.

Yeah, sorry about these missing examples and detailed documentation. I
wanted to get an RFC out quickly and probably underestimated how
important it is even for reviewing an RFC.

However, I think keeping this information only in the commit messages
isn't right either. Do we have any of the schema syntax documented
anywhere? Should I create something in docs/(specs/)?

> [Without reading the patch yet]
> I take it the 'data' of such a union must be completely distinguishable
> by type - the visitor for the 'file' key tries each subtype in turn
> ('BlockOptions' or 'str') until one does not have a parse error.

Not exactly. It asks the visitor for the type of the next item and then
uses a lookup table to get the right discriminator enum value for it.

> And
> this changes the earlier patch that required 'discriminator' to be used
> only when 'base' was also present.  Can it distinguish between two
> structs, or is the union only allowed to have one struct option and all
> other options are primitive types?  Should we consider the use of arrays
> as a union member type?

As the types it uses for lookup are QObject types and all structs map to
QDict, only one struct is allowed.

This makes sense anyway, because allowing different structs would mean
that you potentially get ambiguities when structs look alike (perhaps
only differing in optional fields or something like that).

As soon as we find a good use case for anonymous unions of two structs,
I might change my opinion, though. ;-)

Arrays are a logical extension of this, though I don't have any use for
them now. I wouldn't expect them to be too hard to implement.

> Hmm, I wonder if this new anonymous union can be useful in helping us
> write the self-description of introspection in Amos' patches.

Hm, what's the exact problem there? I know I read that someone (Paolo?)
wrote that it was impossible, but I didn't immediately understand why
and didn't feel like thinking a lot about either.

> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  include/qapi/qmp/qobject.h  |  1 +
> >  include/qapi/visitor-impl.h |  2 ++
> >  include/qapi/visitor.h      |  3 +++
> >  qapi/qapi-visit-core.c      |  9 +++++++++
> >  qapi/qmp-input-visitor.c    | 14 ++++++++++++++
> >  qobject/qjson.c             |  2 ++
> >  scripts/qapi-types.py       | 42 ++++++++++++++++++++++++++++++++++++++++
> >  scripts/qapi-visit.py       | 47 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  scripts/qapi.py             | 28 +++++++++++++++++++++++++++
> >  9 files changed, 148 insertions(+)
> 
> > +++ b/scripts/qapi-visit.py
> > @@ -172,6 +172,49 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, 
> > const char *name, Error **e
> >  ''',
> >                   name=name)
> >  
> > +def generate_visit_anon_union(name, members):
> > +    ret = mcgen('''
> > +
> > +void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, 
> > Error **errp)
> > +{
> > +    Error *err = NULL;
> > +
> > +    if (!error_is_set(errp)) {
> > +        visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), 
> > &err);
> > +        visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, 
> > name, &err);
> > +        switch ((*obj)->kind) {
> > +''',
> > +    name=name)
> > +
> > +    for key in members:
> > +        assert (members[key] in builtin_types
> > +            or find_struct(members[key])
> > +            or find_union(members[key])), "Invalid anonymous union member"
> > +
> > +        ret += mcgen('''
> > +        case %(abbrev)s_KIND_%(enum)s:
> > +            visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
> > +            break;
> > +''',
> > +                abbrev = de_camel_case(name).upper(),
> > +                enum = c_fun(de_camel_case(key),False).upper(),
> > +                c_type=type_name(members[key]),
> > +                c_name=c_fun(key))
> 
> Inconsistent spacing around '='

Thanks, will fix. (It's just copied from generate_visit_union(), though)

> > +
> > +    ret += mcgen('''
> > +        default:
> > +            abort();
> 
> Does this mean I can cause qemu to abort if I pass bogus information on
> the wire?  Using your commit message example,
> 
>     { 'file': false }
> 
> would hit the default case, right?

No, it wouldn't, but that case fails in an interesting way anyway. :-)

We would look up the enum kind in BlockRef_qtypes[QTYPE_BOOL], which
isn't defined and therefore 0. This happens to be the enum value for
BLOCK_REF_KIND_DEFINITION. So I guess what you get is an error message
like "invalid argument type: got bool, expected dict".

> > +++ b/scripts/qapi.py
> > @@ -17,6 +17,21 @@ builtin_types = [
> >      'uint8', 'uint16', 'uint32', 'uint64'
> >  ]
> >  
> > +builtin_type_qtypes = {
> > +    'str':      'QTYPE_QSTRING',
> > +    'int':      'QTYPE_QINT',
> > +    'number':   'QTYPE_QFLOAT',
> > +    'bool':     'QTYPE_QBOOL',
> > +    'int8':     'QTYPE_QINT',
> > +    'int16':    'QTYPE_QINT',
> > +    'int32':    'QTYPE_QINT',
> > +    'int64':    'QTYPE_QINT',
> > +    'uint8':    'QTYPE_QINT',
> > +    'uint16':   'QTYPE_QINT',
> > +    'uint32':   'QTYPE_QINT',
> > +    'uint64':   'QTYPE_QINT',
> 
> What happens if I try to write a union in qapi-schema.json that has both
> a 'int8' and 'uint32' branch?  Since both of those resolve to the
> QTYPE_QINT visitor, does that end up causing the C code to generate a
> switch statement with duplicate labels?

No, the C code gets a lookup table having multiple initialisers for
QTYPE_INT, which means that the last one wins (C11 6.7.9, paragraph 19).

It should be obvious that you're not supposed to do that, so I don't
think it's necessary to assert it in the generator.

> > +}
> > +
> >  def tokenize(data):
> >      while len(data):
> >          ch = data[0]
> > @@ -105,6 +120,7 @@ def parse_schema(fp):
> >          if expr_eval.has_key('enum'):
> >              add_enum(expr_eval['enum'])
> >          elif expr_eval.has_key('union'):
> > +            add_union(expr_eval)
> >              add_enum('%sKind' % expr_eval['union'])
> >          elif expr_eval.has_key('type'):
> >              add_struct(expr_eval)
> > @@ -188,6 +204,7 @@ def type_name(name):
> >  
> >  enum_types = []
> >  struct_types = []
> > +union_types = []
> 
> Is it worth splitting the tracking of the union_types of
> qapi-schema.json into an independent patch?

If you prefer, I can do that. In fact, I considered it myself and didn't
have a strong opinion either way, so I just did something.

> Does it make sense to allow a union type (possibly only an anonymous
> union type) in place of a struct type as a top-level type reference?
> That is, just as we now allow { 'command':'foo', 'data': 'StructType' },
> would it make sense to allow { 'command':bar', 'data': 'AnonUnion' }?

Sounds sensible to me. But do we have a use case for it?

Kevin