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

[Eric Blake <eblake@redhat.com>]

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

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.

[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.  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?

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

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  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 '='

> +
> +    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?

> +++ 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?

> +}
> +
>  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?

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' }?

-- 
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: 621 bytes --]