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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

If we're sufficiently sure the mapping is here to stay.

>> 
>> * 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.

Agree.

>> 
>> * 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.

Will update.

>> 
>>   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/

Will fix.

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

Will update.

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

Yes.

>> 
>> 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/

Will fix.

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

Yes, see [2].  I realized unstable dictionary order could be annoying,
so I added sorting, but forgot to regenerate this example.  Will fix.

>> + "{ \"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.

Accident.  I'll normalize the spacing.

>> +/*
>> + * 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.

Correct.  I forgot that detail, then bought your "must use double quote
in JSON" argument.  It came back later when I wondered why I see a
different order on the wire than in qmp-introspect.c, but by then the
double quote change had been paged out of my working memory.

>                                         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).

Yes, too invasive to be worthwhile: in addition to the generated
marshaller ('gen': false), we'd have to bypass the QMP wire protocol
emitter.

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

For what it's worth, json.dumps() uses double quotes.  I figure it can
be made to use single quotes, but that would be work.  I'm leaning
towards keeping the double quotes.

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

Needs a comment.

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

I intend to give it a try to see whether it turns out 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.

Correct.  I didn't, because I ran out of time.

>> +        ret = '[' + ', '.join(elts) + ' ]'
>
> Here's where I was wondering if you want '[ '.

Yes.

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

You're right.

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

Yes.  I prefer honest assert to untestable code.

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

I'll drop it.  If a caller wants {}, it can pass {}.

>> +
>> +    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'.

Will fix.

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

I want to review the introspection schema systematically for sorting
opportunities.

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

Will fix.

> Overall, still looking fairly close to the end product.

Thanks!