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

[Eric Blake]

On 08/04/2015 09:58 AM, Markus Armbruster wrote:
> Caution, rough edges.
> 
> qapi/introspect.json defines the introspection schema.  It should do
> for uses other than QMP.
> FIXME it's almost entirely devoid of comments.
> 

That part explains why this is still RFC.

> 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.
> 
> Introspection lowers away a number of schema details:
> 
> * The built-in types are declared with their JSON type.
> 
>   TODO Should we map all the integer types to just int?

Is it worth squashing that fix (patch 31) into this one?

> 
> * 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.
> 
>   Example: a simple union implicitly defines an enumeration type for
>   its discriminator.

The names all change if we go with patch 32; but that one should stay
separate.

> 
> * All type references are by name.
> 
> * Base types are flattened.
> 
> * The top type (named 'any') can hold any value.
> 
> * The struct and union types are generalized into an object type.
> 
> * Commands take a single argument and return a single result.
> 
>   Dictionary argument/result or list result is an implicit type
>   definition.

Dictionary result is not possible, due to earlier changes.

> 
>   The empty object type is used when a command takes no arguments or
>   produces no results.
> 
>   The argument is always of object type, but the introspection schema
>   doesn't reflect that.
> 
>   The 'gen': false directive is omitted as implementation detail.
> 
>   The 'success-response' directive is ommitted as well for now, even

s/ommitted/omitted/

>   though it's not an implementation detail.
> 
> * Events carry a single data value.
> 
>   Implicit type definition and empty object type use, just like for
>   commands.
> 
>   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
> 
>   Instead of a mandatory "optional" flag, we have an optional default.
>   No default means mandatory, default null means optional without
>   default value.  Non-null is available for optional with default.
> 
>   Alternate members can't have defaults, but the introspection schema
>   doesn't reflect that.

This sentence is no longer necessary, now that alternates have their own
QAPI representation.

> 
> * Clients should *not* look up types by name, because type names are
>   not ABI.  Look up the command or event you're interested in, then
>   follow the references.
> 
>   TODO Should we hide the type names to eliminate the temptation?
> 
> 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.
> 
> FIXME it can generate awfully long lines

Worth repeating this FIXME in the code?

> 
> A new test-qmp-input-visitor test case feeds its result for both
> tests/qapi-schema/qapi-schema-test.json and qapi-schema.json to a
> QmpInputVisitor to verify it actually conforms to the schema.
> 
> New QMP command query-schema takes its return value from that
> variable.  Command documentation is incomplete, and marked FIXME.  Its
> reply is some 85KiBytes for me right now.
> 
> If this turns out to be too much, we have a couple of options:
> 
> * We can use shorter names in the JSON.  Not the QMP style.
> 
> * Optionally return the sub-schema for commands and events given as
>   arguments.
> 
>   Right now qmp_query_schema() sends the string literal computed by
>   qmp-introspect.py.  To compute sub-schema at run time, we'd have to
>   duplicate parts of qapi-introspect.py in C.  Unattractive.
> 
> * Let clients cache the output of query-schema.
> 
>   It changes only on QEMU upgrades, i.e. rarely.  Provide a command
>   query-schema-hash.  Clients can have a cache indexed by hash, and
>   re-query the schema only when they don't have it cached.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---

Lack of in-place documentation is still the biggest reason I'm not
comfortable with R-b yet (although this round is a bit further along
than v2 was); but there's also a bug that you need to fix on the
introspection for simple unions [1].

> +++ b/docs/qapi-code-gen.txt

> +Example:
> +
> +    $ python scripts/qapi-event.py --output-dir="qapi-generated"

s/event/introspect/

> +    --prefix="example-" example-schema.json
> +    $ cat qapi-generated/example-qmp-introspect.c
> +[Uninteresting stuff omitted...]
> +
> +    const char example_qmp_schema_json[] = "["
> +        "{ \"arg-type\": \":empty\", \"name\": \"MY_EVENT\", \"meta-type\": 
> \"event\" }, "
> +        "{ \"meta-type\": \"builtin\", \"name\": \"int\", \"json-type\": 
> \"int\" }, "

Any reason our poor-man's json formatter doesn't output dictionaries in
any particular order?  It's particularly odd that 'meta-type' and 'name'
switch places across rows.  I'm not sure if an OrderedDict and/or
strategic call to sort() keys would make the introspection nicer for
humans.  Also, forcing a particular order means that we will be
guaranteed identical output regardless of python version or other random
hash factors that might otherwise render the generated file differently
across machines.  On the other hand, the end result doesn't violate JSON
and shouldn't affect machine parsing, so it's not a reason to hold up
the patch.  See also [2]

> +        "{ \"meta-type\": \"builtin\", \"name\": \"str\", \"json-type\": 
> \"string\" }, "
> +        "{ \"meta-type\": \"object\", \"name\": \":empty\", \"members\": [ ] 
> }, "
> +        "{ \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\", 
> \"members\": [{ \"type\": \"UserDefOne\", \"name\": \"arg1\" } ] }, "

Why a space after { but not after [, particularly since there is also a
space before ] and }?  Is it so you don't generate "[  ]" for :empty?
But JSON doesn't care about amount of whitespace, so it is not fatal.

> +/*
> + * Minor hack: generated marshalling suppressed for this command
> + * ('gen': false in the schema) so we can parse the JSON string
> + * directly into QObject instead of first parsing it with
> + * visit_type_SchemaInfoList() into a SchemaInfoList, then marshal it
> + * to QObject with generated output marshallers, every time.  Instead,
> + * we do it in test-qmp-input-visitor.c, just to make sure
> + * qapi-introspect.py's output actually conforms to the schema.
> + */
> +static void qmp_query_schema(QDict *qdict, QObject **ret_data, Error **errp)
> +{
> +    *ret_data = qobject_from_json(qmp_schema_json);

Huh, I just realized: in v2, I complained that your generated string
used 'quotes' instead of "quotes", because I was afraid the 'quote'
would leak onto the QMP wire and cause grief to strict-JSON clients.
But since we are parsing the string with qobject_from_json() (our own
parser, that understands the extension of 'string'), and then generating
the actual JSON output from QObject, what we pass over the QMP wire
_will_ have "quotes", even if qmp_schema_json did not.  Furthermore,
what is sent on the wire has different spacing (all one single line,
unless you turn on the pretty qmp option), so it really doesn't matter
that you did all the work to use \" inside the generated string, nor
whether you use spacing for legibility. But at least I don't think it is
worth to try and output the generated string directly by skipping the
round trip into qobject and back out to JSON (it might be more
efficient, but this command will not be frequently called, and doing the
round trip avoids us having to worry about consistency issues).

So if it makes v4 easier, you could output "'string'" rather than
"\"string\"" for a slightly smaller generated file.  But since you've
already done the work for escaping ", I'm also fine with the style used
in v3.

> +++ b/qapi/introspect.json

> +{ 'struct': 'SchemaInfoObject',
> +  'data': { 'members': [ 'SchemaInfoObjectMember' ],
> +            '*tag': 'str',
> +            '*variants': [ 'SchemaInfoObjectVariant' ] } }

I take it that either tag and variants will both be omitted, or both be
present.  No clean way to represent that in the schema, though.

> +++ b/scripts/qapi-commands.py
> @@ -260,7 +260,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>          self.defn = None
>          self.regy = None
>          self.visited_ret_types = None
> -    def visit_begin(self):
> +    def visit_begin(self, schema):

You already mentioned that you ran out of time in v3 to float this to an
earlier patch; maybe v4 will be better?


> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,172 @@
> +#
> +# 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.
> +# See the COPYING file in the top-level directory.
> +
> +from qapi import *
> +
> +# Caveman's json.dumps() replacement (we're stuck at 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]

No sorting here makes sense (the json-to-string converter can't know
whether we are using arrays as sets, vs. in the usual JSON semantics of
order-preserving).  Thus, if we intend to sort things like enum values
or object members by name, that sorting has to be done prior to feeding
it to this pretty-printer.

> +        ret = '[' + ', '.join(elts) + ' ]'

Here's where I was wondering if you want '[ '.

> +    elif isinstance(obj, dict):
> +        elts = ['"%s": %s' % (key, to_json(obj[key], level + 1))
> +                for key in sorted(obj.keys())]

[2] Sorting here makes sense, except that I'm not sure what happened to
the sorting!  Oh, I see - you captured your docs/qapi-code-gen.txt
_before_ adding the sort; because when I reran the steps in the docs,
the dictionary keys for "MY_EVENT" correctly list "meta-type" before
"name", and "json-type" before "meta-type", contrary to what you pasted
in the docs.

> +        ret = '{ ' + ', '.join(elts) + ' }'
> +    else:
> +        assert False                # not implemented

This would be number and boolean types.  We would need to implement it
when we add support for defaults, but that's down the road.

> +    def _gen_json(self, name, mtype, obj={}):
> +        obj['name'] = name
> +        obj['meta-type'] = mtype
> +        self.jsons.append(obj)

Why is 'obj' given a default here, when all callers pass a non-empty dict?

> +
> +    def _gen_variants(self, tag_name, variants):
> +        return { 'tag': tag_name or 'kind',

[1] Bug mentioned above.  The QMP wire format for a simple union tag is
'type', not 'kind'.

> +                 'variants': [self._gen_variant(v) for v in variants] }

Do you want to add a sort() on variant 'case's here?

> +
> +    def visit_enum_type(self, name, info, values):
> +        self._gen_json(name, 'enum', { 'values': values })

Do you want to add a sort() on values here?

> +
> +    def visit_array_type(self, name, info, element_type):
> +        self._gen_json(name, 'array',
> +                       { 'element-type': self._use_type(element_type) })
> +
> +    def visit_object_type_flat(self, name, info, members, variants):
> +        obj = { 'members': [self._gen_member(m) for m in members] }

Do you want to add a sort() on member 'name's here?

> +        if variants:
> +            obj.update(self._gen_variants(variants.tag_name,
> +                                          variants.variants))
> +        self._gen_json(name, 'object', obj)
> +
> +    def visit_alternate_type(self, name, info, variants):
> +        self._gen_json(name, 'alternate',
> +                       { 'members': [ { 'type': self._use_type(m.type) }
> +                                      for m in variants.variants] })

Do you want to add a sort() on member 'type's here?

> +++ b/tests/test-qmp-input-strict.c

> +
> +static void test_validate_qmp_introspect(TestInputVisitorData *data,
> +                                           const void *unused)
> +{

Indentation is off.

Overall, still looking fairly close to the end product.

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

signature.asc
Description: OpenPGP digital signature