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

[Markus Armbruster]

Michael Roth <address@hidden> writes:

> Quoting Markus Armbruster (2015-09-07 05:16:41)
>> qapi/introspect.json defines the introspection schema.  It's designed
>> for QMP introspection, but should do for similar uses, such as QGA.
>> 
>> 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, and makes
>> implicit things explicit:
>> 
>> * The built-in types are declared with their JSON type.
>> 
>>   TODO Should we map all the integer types to just int?
>> 
>> * Implicit type definitions are made explicit, and given
>>   auto-generated names:
>> 
>>   - Array types, named by appending "List" to the name of their
>>     element type, like in generated C.
>> 
>>   - The enumeration types implicitly defined by simple union types,
>>     named by appending "Kind" to the name of their simple union type,
>>     like in generated C.
>> 
>>   - Types that don't occur in generated C.  Their names start with ':'
>>     so they don't clash with the user's names.
>> 
>> * All type references are by name.
>> 
>> * The struct and union types are generalized into an object type.
>> 
>> * Base types are flattened.
>> 
>> * Commands take a single argument and return a single result.
>> 
>>   Dictionary argument or list result is an implicit type definition.
>> 
>>   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 omitted as well for now, even
>>   though it's not an implementation detail, because it's not used by
>>   QMP.
>> 
>> * 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
>>   (possible future extension).
>> 
>> * 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?
>> 
>> New generator scripts/qapi-introspect.py computes an introspection
>> value for its input, and generates a C variable holding it.
>> 
>> It can generate awfully long lines.  Marked TODO.
>
> Would be good if we could get this is for initial merge to help
> with debugging if anything pops up.

Unlikely, to be honest.  Patches depending on this series have been
piling up.  I'd be okay with delaying follow-up QAPI work some more, but
not work on other subsystems, especially not user-visible goodies.

Work-arounds:

* Take the C string from qmp-introspect.c, unquote it to get actual
  string contents, feed it to your favourite JSON pretty printer.  I've
  used Python's pprint.

* Run query-qmp-schema, feed the result to your favourite JSON pretty
  printer.

* Likewise, using the QMP monitor's integrated pretty printer, e.g.
  with --qmp-pretty.

>> 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-qmp-schema takes its return value from that
>> variable.  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-qmp-schema.
>> 
>>   It changes only on QEMU upgrades, i.e. rarely.  Provide a command
>>   query-qmp-schema-hash.  Clients can have a cache indexed by hash,
>>   and re-query the schema only when they don't have it cached.  Even
>>   simpler: put the hash in the QMP greeting.
>
> Would probably be easier for management to build up their own structure
> for querying caps, so I think a computed hash seems best. But I don't
> think either is something that couldn't be added later if need be.

I also think a hash is the way to go, and I'd like to provide one early,
but I don't want to delay this series for it.

[...]
>> diff --git a/monitor.c b/monitor.c
>> index e083716..ddf6cb5 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -74,6 +74,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 */
>> @@ -928,6 +929,20 @@ EventInfoList *qmp_query_events(Error **errp)
>>      return ev_list;
>>  }
>> 
>> +/*
>> + * 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);
>> +}
>
> What if we still did 'gen': false, manually ran it through input/output
> visitors, but then cached the result? I think the unit test approach
> is reasonable, but currently we only test the input visitor, whereas
> QmpOutputVisitor would come into play for the wire response if we
> ever moved to using a C-based schema visitor (for querying individual
> fields for instance)

Could be explored in a follow-up patch.

>> +
>>  /* 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 d874c11..8c77220 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..5b4713b
>> --- /dev/null
>> +++ b/qapi/introspect.json
>> @@ -0,0 +1,276 @@
>> +# -*- Mode: Python -*-
>> +#
>> +# QAPI/QMP introspection
>> +#
>> +# Copyright (C) 2015 Red Hat, Inc.
>> +#
>> +# Authors:
>> +#  Markus Armbruster <address@hidden>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +# See the COPYING file in the top-level directory.
>> +
>> +##
>> +# @query-qmp-schema
>> +#
>> +# Command query-qmp-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.
>> +#
>> +# However, the SchemaInfo can't reflect all the rules and restrictions
>> +# that apply to QMP.  It's interface introspection (figuring out
>> +# what's there), not interface specification.  The specification is in
>> +# the QAPI schema.
>
> Thanks, the clarification here and in the documentation addresses my
> concerns.

Thanks for making me improve the docs :)

>> +#
>> +# 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.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'query-qmp-schema',
>> +  'returns': [ 'SchemaInfo' ],
>> +  'gen': false }                # just to simplify qmp_query_json()
>> +
>> +##
>> +# @SchemaMetaType
>> +#
>> +# This is a @SchemaInfo's meta type, i.e. the kind of entity it
>> +# describes.
>> +#
>> +# @builtin: a predefined type such as 'int' or 'bool'.
>> +#
>> +# @enum: an enumeration type
>> +#
>> +# @array: an array type
>> +#
>> +# @object: an object type (struct or union)
>> +#
>> +# @alternate: an alternate type
>> +#
>> +# @command: a QMP command
>> +#
>> +# @event: a QMP event
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'enum': 'SchemaMetaType',
>> +  'data': [ 'builtin', 'enum', 'array', 'object', 'alternate',
>> +            'command', 'event' ] }
>> +
>
>
> Small nit, don't we usually put type definitions before the
> commands that reference them?

Yes, we do, but I've never been a fan of slavish define before use.  It
tends to produce documents I need to read partly backwards.  I prefer to
order things in a way that optimizes readability in strict forward
direction.

We had generator bugs around forward references, but I fixed them, and
added test cases (commit 8c3f8e7).