[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions |
Date: |
Thu, 11 Jul 2013 13:47:37 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
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 <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 '='
> +
> + 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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs, (continued)
- [Qemu-devel] [RFC PATCH 05/11] qapi: Add visitor for implicit structs, Kevin Wolf, 2013/07/09
- [Qemu-devel] [RFC PATCH 06/11] qapi: Flat unions with arbitrary discriminator, Kevin Wolf, 2013/07/09
- [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten(), Kevin Wolf, 2013/07/09
- [Qemu-devel] [RFC PATCH 07/11] qapi: Add consume argument to qmp_input_get_object(), Kevin Wolf, 2013/07/09
- [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions, Kevin Wolf, 2013/07/09
- Re: [Qemu-devel] [RFC PATCH 08/11] qapi: Anonymous unions,
Eric Blake <=
- [Qemu-devel] [RFC PATCH 10/11] block: Allow "driver" option on the top level, Kevin Wolf, 2013/07/09
- [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command, Kevin Wolf, 2013/07/09
Re: [Qemu-devel] [RFC PATCH 00/11] qapi changes in preparation for blockdev-add, Laszlo Ersek, 2013/07/12