Re: [Qemu-devel] [PATCH RFC 19/19] qapi: New QMP command query-schema for QMP schema introspection

[Eric Blake <eblake@redhat.com>]

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

On 04/02/2015 11:29 AM, Markus Armbruster wrote:
> Caution, rough edges.
> 
> qapi/introspect.json defines the introspection schema.  It should do
> for uses other than QMP.

That is, QGA should also be able to reuse it for introspection.

> FIXME it's almost entirely devoid of comments.

Yeah, but it's only RFC quality, and if we alter design you don't have
to worry about lengthy comments to keep in sync :)  Of course, the final
version ought to have good comments.

> 
> The introspection schema does not reflect all the rules and
> restrictions that apply to QAPI schemata.  A valid QAPI schema has an
> introspection value conforming to the introspection schema, but the
> converse is not true.

I can live with that.  Introspection is output-only (we aren't creating
new runtime commands by using introspection descriptions as input, so it
doesn't matter if the introspection schema permits more than qapi will
ever generate).

> 
> Introspection lowers away a number of schema details:
> 
> * The builtin types are declared with their JSON type.
> 
>   TODO should we map all the integer types to just int?

Or even have introspection output two things: type ('int' for all JSON
integers, regardless of range) and range ([0,255] for 'uint8').  I could
live with that (ideally, knowing the range will help libvirt avoid
passing in too-large-a-value to a parameter it introspected; but right
now the idea is that most of libvirt's introspection will be "does
something exist" not "what range does it support", and that libvirt will
already have hard-coded knowledge of "if it exists, it is a uint8"
without having to ask introspection for the type libvirt will supply).

> 
> * Implicit type definitions are made explicit, and given
>   auto-generated names.  These names start with ':' so they don't
>   clash with the user's names.

Hopefully that works.  Although having to parse for ':' is an argument
that we are packing multiple pieces of information in one JSON
name/value, which sometimes means we should have instead supplied two
different name/values for the two different pieces of information. Oh
well, I'll see more when I get into the schema part.

Maybe we could also tweak the generator to put implicit names in the
leading '_' namespace (we already outlaw use of leading '_' in all but
downstream names, and require downstream names to use double '__', so we
could easily identify '_generated' and '___generated' as internal vs.
'regular' and '__downstream' as user-supplied; it would free up current
places where the qapi user cannot supply certain names. But not for this
series).

> 
>   Example: a simple union implicitly defines an enumeration type for
>   its discriminator.
> 
> * All type references are by name.

Fine if the type is named; but what happens if the type is anonymous
inline (as in many commands?)  Also, in my nested struct cleanups, I
found several places where it would be nice to patch the generator to
support more anonymous inline types, such as in:

{ 'enum': 'MyEnum', 'data': [ 'one', 'two' ] }
{ 'union': 'Foo', 'base': { 'switch': 'MyEnum', 'name': 'str' },
  'discriminator': 'switch', data': {
    'one': { 'value': 'int' },
    'two': { 'default': 'int' } } }

It would even work for our long-hand of optional arguments:
{ 'command': 'foo', 'data': {
  'bar': { 'type': { 'name': 'str', 'value': 'int' },
           'optional': true } } }

I guess we can always go with anonymous inline types resulting in an
implicit named type creation, and refer to that name, if cramming an
anonymous type into the schema is too hard.

> 
> * Base types are flattened.

That's fine - we are introspecting what can be sent on the wire, and
don't care what types were used in the qapi to arrive at that point.

> 
> * The top type (named '**') can hold any value.
> 
>   It can currently occur only in commands, but the introspection
>   schema doesn't reflect that.
> 
> * The struct and union types are generalized into an object type.

I think you mentioned ideas on that in reviewing my v5 thread; they
sounded reasonable at the time, so we'll see when I actually review the
schema.

> 
> * Commands take a single argument and return a single result.
> 
>   Dictionary argument/result or list result is an implicit type
>   definition.
> 
>   Missing argument/result is an implicit definition of the empty
>   object.
> 
>   The argument is always of object type, but the introspection schema
>   doesn't reflect that.
> 
>   The 'gen': false directive is omitted as implementation detail.

Yep, that's fine. QAPI drives internal details that don't affect the
wire format, and it's fine that introspection doesn't have to expose them.

> 
> * Events carry a single data value.
> 
>   Implicit type definition as above.
> 
>   The value is of object type, but the introspection schema doesn't
>   reflect that.
> 
> * Types not used by commands or events are omitted.
> 
>   Indirect use counts as use.
> 
> * Optional members have a default, which can only be null right now

Even for integral members?

> 
>   Instead of a mandatory "optional" flag, we have an optionial

s/optionial/optional/

>   default.  No default means mandatory, default null means optional
>   without default value.  Non-null is available for optional with
>   default.
> 
> TODO much of the above should go into docs.
> 
> New generator scripts/qapi-introspect.py computes an introspection
> value for its input, and generates a C variable holding it.
> Pythonistas may find it ugly and/or clumsy.

I'm not one, so my review will (happily?) paper over the clumsy usage.

> 
> FIXME it can generate awfully long lines

Not the first of our generators with that problem.  But even tougher if
you are generating C strings for highly-complex types.

> 
> The schema generation proper is pretty trivial.  Much of the
> qapi-introspect.py's code actually does something else: convert the
> output of parse_schema() into a more usable data structure, lowering
> away uninteresting stuff.  This should really be done in qapi.py, so
> the other generators can use it, too.

And we may want to start by breaking that out into separate commits.

> 
> For testing, I feed it tests/qapi-schema/qapi-schema-test.json, but no
> more.

That's actually a good test - it tries to cover as much of our generator
as possible, without too many repetitions of types of objects.

> 
> New QMP command query-schema parses its return value from that
> variable.  Its reply is less than 75KiBytes right now.

A bit heavy; implies that we might want it to support optional
filtering. But that can be added on top.

> 
> A compile time test that the value can be parsed would be nice.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  .gitignore                 |   1 +
>  Makefile                   |   9 +-
>  Makefile.objs              |   1 +
>  monitor.c                  |   8 +
>  qapi-schema.json           |   3 +
>  qapi/introspect.json       |  72 ++++++++
>  qmp-commands.hx            |  16 ++
>  scripts/qapi-introspect.py | 430 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore           |   1 +
>  tests/Makefile             |   8 +-

No docs/qapi-code-gen.txt changes? (Oh right, you said so, for the RFC...)

>  10 files changed, 547 insertions(+), 2 deletions(-)
>  create mode 100644 qapi/introspect.json
>  create mode 100644 scripts/qapi-introspect.py

Should it be 755?  Oh, none of the other qapi*.py are.

> +++ b/monitor.c
> @@ -73,6 +73,7 @@
>  #include "block/qapi.h"
>  #include "qapi/qmp-event.h"
>  #include "qapi-event.h"
> +#include "qmp-introspect.h"
>  #include "sysemu/block-backend.h"
>  
>  /* for hmp_info_irq/pic */
> @@ -997,6 +998,13 @@ EventInfoList *qmp_query_events(Error **errp)
>      return ev_list;
>  }
>  
> +static int qmp_query_schema(Monitor *mon, const QDict *qdict,
> +                            QObject **ret_data)
> +{
> +    *ret_data = qobject_from_json(qmp_schema_json);
> +    return 0;

Deceptively simple - it means everything is known at compile-time (no
run-time filtering for conditionally implemented commands; might become
important later on for listing only whitelisted QGA commands, for example?)

> +}
> +
>  /* set the current CPU defined by the user */
>  int monitor_set_cpu(int cpu_index)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 6b280b7..0efeb80 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -14,6 +14,9 @@
>  # Tracing commands
>  { 'include': 'qapi/trace.json' }
>  
> +# QAPI introspection
> +{ 'include': 'qapi/introspect.json' }
> +
>  ##
>  # LostTickPolicy:
>  #
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> new file mode 100644
> index 0000000..87de856
> --- /dev/null
> +++ b/qapi/introspect.json
> @@ -0,0 +1,72 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI introspection
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# Authors:
> +#  Markus Armbruster <armbru@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +{ 'enum': 'SchemaMetaType',
> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
> +            'command', 'event' ] }

Yep, definitely depends on my work going in first.

'builtin' is not an English word; do we want to go with the longer
"built-in", or is it not worth it?

> +
> +{ 'type': 'SchemaInfoBase',
> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
> +
> +{ 'enum': 'JSONPrimitiveType',
> +  'data': [ 'str', 'int', 'number', 'bool', 'null' ] }
> +
> +{ 'type': 'SchemaInfoBuiltin',
> +  'data': { 'json-type': 'JSONPrimitiveType' } }
> +
> +{ 'type': 'SchemaInfoEnum',
> +  'data': { 'values': ['str'] } }

At one point, one thread suggested that we might want to allow QAPI to
support enums with fixed values, as in:

'data': [ {'one': 1}, {'three': 3} ]

I guess returning an array of 'str' for now is safe; because we could
always increase introspection to return an alternate between 'str' and a
formal struct down the road if the user needs to know about enums with
values that are guaranteed (vs. the usual enum that is name-association
only with no guaranteed value).

> +
> +{ 'type': 'SchemaInfoArray',
> +  'data': { 'element-type': 'str' } }

We currently don't allow array of anonymous types, but looks like your
implicit type naming would cover that.

> +
> +{ 'alternate': 'Value',
> +  'data': { 'int': 'int', 'number': 'number', 'str': 'str', 'bool': 'bool' } }

Nothing for objects?...

> +
> +{ 'type': 'SchemaInfoObjectMember',
> +  'data': { 'name': 'str', 'type': 'str', '*default': 'Value' } }
> +# @default's type must match @type
> +# can only default simple types, not objects or arrays

but then again, that's what you said, in a rare comment.

> +
> +{ 'type': 'SchemaInfoObjectVariant',
> +  'data': { 'case': 'str',
> +            'members': [ 'SchemaInfoObjectMember' ] } }
> +
> +{ 'type': 'SchemaInfoObject',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
> +            '*discriminator': 'str',
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }

I guess since the discriminator is always a JSON string, that the 'str'
here is the type name ('str' or an enum name)?  Or is it the
discriminator member name ('type' on simple unions, whatever
'discriminator' was on flat unions)?  We probably want BOTH pieces of
information, always, particularly if I finish my work on extending
simple unions to have an enum-type discriminator.  On the other hand,
the 'variants' array lists all of the branches, which means I can
reconstruct the enum values (or even the open-coded 'str' values)
without caring whether the discriminator was typed.

So after all that, I guess you are using it as the discriminator member
name and NOT its type.

> +
> +{ 'type': 'SchemaInfoAlternate',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ] } }
> +
> +{ 'type': 'SchemaInfoCommand',
> +  'data': { 'args': 'str', 'returns': 'str' } }

We could always use an alternate to return either a 'str' or a
'SchemaInfoObject' to represent anonymous inline types without having to
go through an implicit generated name.  If that makes any more sense.

> +
> +{ 'type': 'SchemaInfoEvent',
> +  'data': { 'data': 'str' } }

Likewise.

> +
> +{ 'union': 'SchemaInfo',
> +  'base': 'SchemaInfoBase',
> +  'discriminator': 'meta-type',
> +  'data': {
> +      'builtin': 'SchemaInfoBuiltin',
> +      'enum': 'SchemaInfoEnum',
> +      'array': 'SchemaInfoArray',
> +      'object': 'SchemaInfoObject',
> +      'alternate': 'SchemaInfoAlternate',
> +      'command': 'SchemaInfoCommand',
> +      'event': 'SchemaInfoEvent' } }
> +
> +{ 'command': 'query-schema',
> +  'returns': [ 'SchemaInfo' ],
> +  'gen': false }

Overall, looks pretty nice.  But if we reuse this file for both QMP and
QGA, then maybe the 'command' should be in the parent file that includes
this (so that this file is just everything through 'SchemaInfo' to
declare the types, but leaves QGA free to name its command different
than QMP, if desired)

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7f68760..27ab209 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2039,6 +2039,22 @@ EQMP
>      },
>  
>  SQMP
> +query-schema
> +------------
> +
> +Return the QMP schema.
> +
> +FIXME explain
> +
> +EQMP
> +
> +    {
> +        .name       = "query-schema",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_query_schema,
> +    },
> +
> +SQMP
>  query-chardev
>  -------------
>  
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> new file mode 100644
> index 0000000..c09276b
> --- /dev/null
> +++ b/scripts/qapi-introspect.py

I stopped reviewing here for today.

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