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

[Eric Blake]

On 09/03/2015 08:30 AM, Markus Armbruster wrote:
> qapi/introspect.json defines the introspection schema.  It's designed
> for QMP introspection, but should do for similar uses, such as QGA.

[review in this sub-thread; for comments on 'int' munging or other
followups, see other subthread]

There is at least one definite bug (see multiple references below to
[1]), and several ideas for cleanups, but in general, I think that the
remaining changes are going to be small enough that I'd probably be okay
if v5 started life with:
Reviewed-by: Eric Blake <address@hidden>
(Of course, you have every right to not add the R-b, especially if you
want me to do another close review :)

> 
> 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.
> 
> 
>   The empty object type is used when a command takes no arguments or
>   produces no results.

We had discussed whether to expose the empty type in a separate patch,
to reduce the size of this patch; but at this point in the game, I'm
happy to keep it as-is (the faster we get v5 out for review and into the
tree, the faster we can process the backlog of followup patches).

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---


> +++ b/docs/qapi-code-gen.txt
> @@ -494,13 +494,195 @@ Resulting in this JSON object:
>    "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>  
>  
> +== Client JSON Protocol introspection ==
> +
> +Clients of a Client JSON Protocol commonly need to figure out what
> +exactly the server (QEMU) supports.
> +
> +For this purpose, QMP provides introspection via command query-schema.

Will need tweaking to the agreed-on name query-qmp-schema...

> +QGA currently doesn't support introspection.

... (and we'd obviously want qga to eventually gain a matching
counterpart, possibly 'guest-schema' or 'guest-get-schema', to match
prevailing naming in qga/qapi-schema.json).

> +
> +query-schema returns a JSON array of SchemaInfo objects.  These
> +objects together describe the wire ABI, as defined in the QAPI schema.

Maybe an additional sentence here, perhaps along the lines of:

Note that the wire format cannot express everything; it is not designed
to show semantic constraints (such as when exactly one of a pair of
mutually-exclusive optional members must be present, or when an integral
value has specific limitations on valid values).  Introspection allows
an application to know if a feature is present, but the application must
obey the qapi documentation to properly interact with that feature.

> +
> +Like any other command, query-schema is itself defined in the QAPI
> +schema, along with the SchemaInfo type.  This text attempts to give an
> +overview how things work.  For details you need to consult the QAPI
> +schema.
> +
> +SchemaInfo objects have common members "name" and "meta-type", and
> +additional variant members depending on the value of meta-type.
> +
> +Each SchemaInfo object describes a wire ABI entity of a certain
> +meta-type: a command, event or one of several kinds of type.
> +
> +SchemaInfo for entities defined in the QAPI schema have the same name
> +as in the schema.  This is the case for all commands and events, and
> +most types.

And hopefully patch 32 munges this text to match the simplification made
there (I haven't checked yet).

> +
> +Command and event names are part of the wire ABI, but type names are
> +not.  Therefore, looking up a type by its name in the QAPI schema is
> +wrong.  Look up the command or event, then follow references by name.
> +
> +QAPI schema definitions not reachable that way are omitted.
> +
> +The SchemaInfo for a command has meta-type "command", and variant
> +members "arg-type" and "ret-type".  The arguments member clients pass

I found it easier to read with:
s/arguments/arguments that/

> +with a command on the wire must conform to the type named by
> +"arg-type", which is always an object type.  The return value the

Likewise s/value/value that/

> +server passes in a success response conforms to the type named by
> +"ret-type".
> +

[not for this patch. Thinking aloud: I wonder if QGA's success-response
could be coded in by having 'ret-type':null as a way of saying the
command will have no response if it is successful; perhaps a bit nicer
than exposing an additional boolean flag member]

> +If the command takes no arguments, "arg-type" names an object type
> +without members.  Likewise, if the command returns nothing, "ret-type"
> +names an object type without members.
> +
> +Example: the SchemaInfo for command query-schema
> +
> +    { "name": "query-schema", "meta-type": "command",
> +      "arg-type": ":empty", "ret-type": "SchemaInfoList" }
> +
> +    Type ":empty" is an object type without members, and type
> +    "SchemaInfoList" is the array of SchemaInfo type.
> +
> +The SchemaInfo for an event has meta-type "event", and variant member
> +"arg-type".  The data member the server passes with an event conforms

again, reads a little better with s/member/member that/

> +to the type named by "arg-type".  It is always an object type.
> +
> +If the event carries no additional information, "arg-type" names an
> +object type without members.  The event may not have a data member on
> +the wire then.
> +
> +Each command or event defined with dictionary-valued 'data' in the
> +QAPI schema implicitly defines an object type called ":obj-NAME-arg",
> +where NAME is the command or event's name.
> +
> +Example: the SchemaInfo for EVENT_C from section Events
> +
> +    { "name": "EVENT_C", "meta-type": "event",
> +      "arg-type": ":obj-EVENT_C-arg" }
> +
> +    Type ":obj-EVENT_C-arg" is an implicitly defined object type with
> +    the two members from the event's definition.
> +
> +The SchemaInfo for struct and union types has meta-type "object".
> +
> +The SchemaInfo for a struct type has variant member "members".

Worth mentioning that "members" may be empty, for an empty struct?

> +
> +The SchemaInfo for a union type additionally has variant members "tag"
> +and "variants".

Worth mentioning that "members" will never be empty, because it contains
at least the member also referenced in "tag"?

> +
> +"members" is a JSON array describing the object's common members.
> +Each element is a JSON object with members "name" (the member's name),
> +"type" (the name of its type), and optionally "default".  The member
> +is optional if "default" is present.  Currently, "default" can only
> +have value null.  Other values are reserved for future extensions.
> +
> +Example: the SchemaInfo for MyType from section Struct types
> +
> +    { "name": "MyType", "meta-type": "object",
> +      "members": [
> +          { "name": "member1", "type": "str" },
> +          { "name": "member2", "type": "int" },
> +          { "name": "member3", "type": "str", "default": null } ] }
> +
> +"tag" is the name of the common member serving as type tag.
> +"variants" is a JSON array describing the object's variant members.
> +Each element is a JSON object with members "case" (the value of type
> +tag this element applies to) and "type" (the name of an object type
> +that provides the variant members for this type tag value).
> +
> +Example: the SchemaInfo for flat union BlockdevOptions from section
> +Union types

Hmm, the "Union types" section has two mentions of BlockdevOptions; and
this example matches the second. It may help readability if we rename
one of the two examples to be distinct (preferably the first, since it
doesn't match actual QMP).  I guess the phrase "flat union
BlockdevOptions" is sufficient to make it obvious that we are referring
to the second usage, but it is subtle.

> +
> +    { "name": "BlockdevOptions", "meta-type": "object",
> +      "members": [
> +          { "name": "driver", "type": "BlockdevDriver" },
> +          { "name": "readonly", "type": "bool"} ],
> +      "tag": "driver",
> +      "variants": [
> +          { "case": "file", "type": "FileOptions" },
> +          { "case": "qcow2", "type": "Qcow2Options" } ] }
> +
> +Note that base types are "flattened": its members are included in the
> +"members" array.
> +
> +A simple union implicitly defines an enumeration type for its implicit
> +discriminator (called "type" on the wire, see section Union types).
> +Such a type's name is made by appending "Kind" to the simple union's
> +name.
> +
> +A simple union implicitly defines an object type for each of its
> +variants.  The type's name is ":obj-NAME-wrapper", where NAME is the
> +name of the name of the variant's type.
> +
> +Example: the SchemaInfo for simple union BlockdevOptions from section
> +Union types

Ah, and here you refer to the other BlockdevOptions. So the point about
a judicious rename may still be warranted.

> +
> +    { "name": "BlockdevOptions", "meta-type": "object",
> +      "members": [
> +          { "name": "kind", "type": "BlockdevOptionsKind" } ],

[1] Ouch.  That should be "name": "type".  I pointed out the bug in v3,
but it looks like it still hasn't been fixed.  Or rather, that our
attempt to fix it wasn't correct.

> +      "tag": "type",
> +      "variants": [
> +          { "case": "file", "type": ":obj-FileOptions-wrapper" },
> +          { "case": "qcow2", "type": ":obj-Qcow2Options-wrapper" } ] }
> +
> +    Enumeration type "BlockdevOptionsKind" and the object types
> +    ":obj-FileOptions-wrapper", ":obj-Qcow2Options-wrapper" are
> +    implicitly defined.
> +
> +The SchemaInfo for an alternate type has meta-type "alternate", and
> +variant member "members".  "members" is a JSON array.  Each element is
> +a JSON object with member "type", which names a type.  Values of the
> +alternate type conform to exactly one of its member types.
> +
> +Example: the SchemaInfo for BlockRef from section Alternate types
> +
> +    { "name": "BlockRef", "meta-type": "alternate",
> +      "members": [
> +          { "type": "BlockdevOptions" },
> +          { "type": "str" } ] }
> +
> +The SchemaInfo for an array type has meta-type "array", and variant
> +member "element-type", which names the array's element type.  Array
> +types are implicitly defined.  An array type's name is made by
> +appending "List" to its element type's name.
> +
> +Example: the SchemaInfo for ['str']
> +
> +    { "name": "strList", "meta-type": "array",
> +      "element-type": "str" }
> +
> +The SchemaInfo for an enumeration type has meta-type "enum" and
> +variant member "values".
> +
> +Example: the SchemaInfo for MyEnum from section Enumeration types
> +
> +    { "name": "MyEnum", "meta-type": "enum",
> +      "values": [ "value1", "value2", "value3" ] }
> +
> +The SchemaInfo for a built-in type has the same name as the type in
> +the QAPI schema (see section Built-in Types).  It has variant member
> +"json-type" that shows how values of this type are encoded on the
> +wire.
> +
> +Example: the SchemaInfo for str
> +
> +    { "name": "str", "meta-type": "builtin", "json-type": "string" }
> +
> +As explained above, type names are not part of the wire ABI.  Not even
> +the names of built-in types.  Clients should examine member
> +"json-type" instead of hard-coding names of built-in types.

Good point (although we may still end up with clients that disobey this,
I will certainly make sure to do this in libvirt's interactions).

> +
> +
>  == Code generation ==
>  
> -Schemas are fed into 3 scripts to generate all the code/files that, paired
> -with the core QAPI libraries, comprise everything required to take JSON
> -commands read in by a Client JSON Protocol server, unmarshal the arguments 
> into
> -the underlying C types, call into the corresponding C function, and map the
> -response back to a Client JSON Protocol response to be returned to the user.
> +Schemas are fed into four scripts to generate all the code/files that,
> +paired with the core QAPI libraries, comprise everything required to
> +take JSON commands read in by a Client JSON Protocol server, unmarshal
> +the arguments into the underlying C types, call into the corresponding
> +C function, and map the response back to a Client JSON Protocol
> +response to be returned to the user.

Doesn't mention introspection; but then again, it also doesn't mention
that it generates the C interface for emitting events.

>  
>  As an example, we'll use the following schema, which describes a single
>  complex user-defined type (which will produce a C struct, along with a list
> @@ -848,3 +1030,37 @@ Example:
>      extern const char *const example_QAPIEvent_lookup[];
>  
>      #endif
> +
> +=== scripts/qapi-introspect.py ===
> +
> +Used to generate the introspection C code for a schema. The following
> +files are created:
> +
> +$(prefix)qmp-introspect.c - Defines a string holding a JSON
> +                            description of the schema.
> +$(prefix)qmp-introspect.h - Declares the above string.
> +
> +Example:
> +
> +    $ python scripts/qapi-introspect.py --output-dir="qapi-generated"
> +    --prefix="example-" example-schema.json
> +    $ cat qapi-generated/example-qmp-introspect.c
> +[Uninteresting stuff omitted...]
> +
> +    const char example_qmp_schema_json[] = "["
> +        "{\"arg-type\": \":empty\", \"meta-type\": \"event\", \"name\": 
> \"MY_EVENT\"}, "
> +        "{\"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\": 
> \"int\"}, "
> +        "{\"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\": 
> \"str\"}, "
> +        "{\"members\": [], \"meta-type\": \"object\", \"name\": \":empty\"}, 
> "
> +        "{\"members\": [{\"name\": \"arg1\", \"type\": \"UserDefOne\"}], 
> \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\"}, "
> +        "{\"members\": [{\"name\": \"integer\", \"type\": \"int\"}, 
> {\"name\": \"string\", \"type\": \"str\"}], \"meta-type\": \"object\", 
> \"name\": \"UserDefOne\"}, "
> +        "{\"arg-type\": \":obj-my-command-arg\", \"meta-type\": \"command\", 
> \"name\": \"my-command\", \"ret-type\": \"UserDefOne\"}]";
> +    $ cat qapi-generated/example-qmp-introspect.h
> +[Uninteresting stuff omitted...]
> +
> +    #ifndef EXAMPLE_QMP_INTROSPECT_H
> +    #define EXAMPLE_QMP_INTROSPECT_H
> +
> +    extern const char example_qmp_schema_json[];
> +
> +    #endif

Overall, a nice documentation addition, and solves one of my missing
checkboxes against v3 :)

> +++ b/qapi-schema.json
> @@ -14,6 +14,9 @@
>  # Tracing commands
>  { 'include': 'qapi/trace.json' }
>  
> +# QAPI introspection
> +{ 'include': 'qapi/introspect.json' }

When we add QGA introspection, we'll want qapi/introspect.json to
contain JUST types, and move the 'command' into qapi-schema.json proper
(that way, qga/qapi-schema.json can also include the same types, then
add its variation on the command).  As it won't matter until we actually
do get to QGA, I'm okay whether you do the hoisting of the 'command' now
or save it for later.

> +++ b/qapi/introspect.json
> @@ -0,0 +1,271 @@

> +
> +##
> +# @query-schema
> +#
> +# Command query-schema exposes the QMP wire ABI as an array of
> +# SchemaInfo.  This lets QMP clients figure out what commands and
> +# events are available in this QEMU, and their parameters and results.
> +#
> +# Returns: array of @SchemaInfo, where each element describes an
> +# entity in the ABI: command, event, type, ...
> +#
> +# Note: the QAPI schema is also used to help define *internal*
> +# interfaces, by defining QAPI types.  These are not part of the QMP
> +# wire ABI, and therefore not returned by this command.

Same as for the docs above: may be worth a paragraph explicitly
mentioning that the wire ABI cannot express everything, such as integer
ranges, or such as semantics where exactly one of two mutually-exclusive
optional members must be present.

...
> +##
> +# @SchemaInfoBase
> +#
> +# Members common to any @SchemaInfo.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoBase',
> +  'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } }
> +
> +##
> +# @SchemaInfo
> +#
> +# @name: the entity's name, inherited from @base.

Interesting way to document @name (I probably would have documented
@base as part of SchemaInfoBase); but it worked out well.  No need to
change it :)

> +# Entities defined in the QAPI schema have the name defined in the schema.
> +# Implicitly defined entities have generated names.  See
> +# docs/qapi-code-gen.txt section "Client JSON Protocol introspection"
> +# for details.
> +#
> +# All references to other SchemaInfo are by name.
> +#
> +# Command and event names are part of the wire ABI, but type names are
> +# not.  Therefore, looking up a type by "well-known" name is wrong.
> +# Look up the command or event, then follow the references.
> +#
> +# @meta-type: the entity's meta type, inherited from @base.
> +#
> +# Additional members depend on the value of @meta-type.
> +#
> +# Since: 2.5
> +##
> +{ 'union': 'SchemaInfo',
> +  'base': 'SchemaInfoBase',
> +  'discriminator': 'meta-type',
> +  'data': {
> +      'builtin': 'SchemaInfoBuiltin',
> +      'enum': 'SchemaInfoEnum',
> +      'array': 'SchemaInfoArray',
> +      'object': 'SchemaInfoObject',
> +      'alternate': 'SchemaInfoAlternate',
> +      'command': 'SchemaInfoCommand',
> +      'event': 'SchemaInfoEvent' } }
> +
> +##
> +# @SchemaInfoBuiltin
> +#
> +# Additional SchemaInfo members for meta-type 'builtin'.
> +#
> +# @json-type: the JSON type used for this type on the wire.

Might be worth repeating the caveat in the earlier docs that @name is
not guaranteed to be stable, so clients should check @json-type.

(especially true if we later decide to expose range information by
adding more builtins or user-defined subtypes of builtins to represent
those ranged integers).

...
> +
> +##
> +# @SchemaInfoEnum
> +#
> +# Additional SchemaInfo members for meta-type 'enum'.
> +#
> +# @values: the enumeration type's values.
> +#
> +# Values of this type are JSON string on the wire.

I'm assuming that if we add sorting of elements in a later patch, we can
better document that guarantee here.

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoEnum',
> +  'data': { 'values': ['str'] } }
> +
> +##
> +# @SchemaInfoArray
> +#
> +# Additional SchemaInfo members for meta-type 'array'.
> +#
> +# @element-type: the array type's element type.
> +#
> +# Values of this type are JSON array on the wire.

Is it worth adding either of these clarifications?

As required by RFC 7159, the order of individual elements with in the
array sent over the wire is assumed to be significant unless the
documented semantics in qapi state otherwise.

While RFC 7159 permits an array to have elements of different types, all
elements of a QMP array should have the same type.

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoArray',
> +  'data': { 'element-type': 'str' } }
> +
> +##
> +# @SchemaInfoObject
> +#
> +# Additional SchemaInfo members for meta-type 'object'.
> +#
> +# @members: the object type's (non-variant) members.
> +#
> +# @tag: #optional the name of the member serving as type tag.  An
> +# element of @members with this name must exist.
> +#
> +# @variants: #optional variant members, i.e. additional members that
> +# depend on the type tag's value.  Present exactly when @tag is
> +# present.
> +#
> +# Values of this type are JSON object on the wire.

Worth adding this clarification?

As documented by RFC 7159, the wire format allows members of the JSON
object to be sent in any order, and requires that members must not be
duplicated.

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoObject',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
> +            '*tag': 'str',
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }
> +
> +##
> +# @SchemaInfoObjectMember
> +#
> +# An object member.
> +#
> +# @name: the member's name, as defined in the QAPI schema.
> +#
> +# @type: the name of the member's type.
> +#
> +# @default: #optional default when used as command parameter.
> +#           If absent, the parameter is mandatory.
> +#           If present, the value must be null.  The parameter is

Maybe:

s/The parameter/On the wire, the parameter/

> +#           optional, and behavior when it's missing is not specified
> +#           here.
> +#           Future extension: if present and non-null, the parameter
> +#        is optional, and defaults to this value.

TAB damage.

> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoObjectMember',
> +  'data': { 'name': 'str', 'type': 'str', '*default': 'any' } }
> +# @default's type must be null or match @type

Is this line still worth keeping, given the more complete explanation
above?  But we do need to use 'any' here, as that is currently the only
way for qapi-generated code to allow for JSON null.

> +
> +##
> +# @SchemaInfoObjectVariant
> +#
> +# The variant members for a value of the type tag.
> +#
> +# @case: a value of the type tag.
> +#
> +# @type: the name of the object type that provides the variant members
> +# when the type tag has value @case.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'SchemaInfoObjectVariant',
> +  'data': { 'case': 'str', 'type': 'str' } }

Do we want to document whether all case values of the tag are guaranteed
to be covered (happens to be true for all current qapi, but is not yet
enforced; although that's one of my planned followup patches is to start
enforcing it - so I guess I can document it at that time if we don't do
it here)

> +##
> +# @SchemaInfoCommand
> +#
> +# Additional SchemaInfo members for meta-type 'command'.
> +#
> +# @arg-type: the name of the object type that provides the command's
> +# parameters.
> +#
> +# @ret-type: the name of the command's result type.
> +#
> +# TODO @success-response (currently irrelevant, because it's QGA, not QMP)

and my idea of using 'ret-type':null may obviate the need for an actual
'success-response' parameter.

> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,174 @@
> +#
> +# QAPI introspection generator
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# Authors:
> +#  Markus Armbruster <address@hidden>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.

No 'or later' clause?  Oh, because your hands are tied: scripts/qapi.py
is GPLv2-only.  Yuck. I wonder if we can fix that; although it may be
hard getting Anthony to explain his original choice.  Separate cleanup,
if at all. [2]

> +# See the COPYING file in the top-level directory.
> +
> +from qapi import *
> +import string
> +
> +# Caveman's json.dumps() replacement (we're stuck at 2.4)

s/2.4/python 2.4/

> +# TODO try to use json.dumps() once we get unstuck
> +def to_json(obj, level=0):
> +    if obj == None:
> +        ret = 'null'
> +    elif isinstance(obj, str):
> +        ret = '"' + obj.replace('"', r'\"') + '"'
> +    elif isinstance(obj, list):
> +        elts = [to_json(elt, level + 1)
> +                for elt in obj]
> +        ret = '[' + ', '.join(elts) + ']'
> +    elif isinstance(obj, dict):
> +        elts = ['"%s": %s' % (key, to_json(obj[key], level + 1))

This relies on 'key' never having any embedded ".  It happens to work
given our naming conventions of what we pass through this dumper, but
might be safer if you used key.replace('"', r'\"') to handle generic
dumping.

> +
> +    def _gen_member(self, member):
> +        ret = {'name': member.name, 'type': self._use_type(member.type)}
> +        if member.optional:
> +            ret['default'] = None
> +        return ret
> +
> +    def _gen_variants(self, tag_name, variants):
> +        return {'tag': tag_name or 'type',

[1] Ouch. Why are we still printing 'kind' as the name for simple
unions?  Oh, it's because tag_name is ALWAYS provided by the visitor, so
the "or 'type'" clause never fires.  I have a pending patch to change
the C code to generate 'type' as the C code name to match the wire ABI;
but until that patch is in, I think a hack solution might be to fix the
visitor to supply None instead of a tag_name for simple unions.  I'll
respond to the appropriate earlier patch.

> +h_comment = '''
> +/*
> + * QAPI/QMP schema introspection
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */

[2] It's interesting that you are following the pattern of
scripts/qapi-commands.py (commit c17d9908); even there, the script
itself was GPLv2-only, while the generated output was LGPLv2+.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature