Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>
> 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 <arm...@redhat.com>
>> +#
>> +# 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!
