Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 13690 bytes --]

On 07/16/2013 04:37 AM, Amos Kong wrote:
> Introduces new monitor command to query QMP schema information,
> the return data is a dynamical and nested dict/list, it contains

s/dynamical/dynamic/

> the useful metadata to help management to check feature support,
> QMP commands detail, etc.
> 
> I added a document for QMP introspection support.
> (docs/qmp-full-introspection.txt)

Yay - docs make a world of difference.

> 
> We need to parse all commands json definition, and generated a

mixing tense ("need" present, "generated" past); for commit messages,
present tense is generally appropriate.  Thus:

s/generated/generate/

> dynamical tree, QMP infrastructure will convert the tree to

s/dynamical/dynamic/

> json string and return to QMP client.
> 
> So here I defined a 'DataObject' type in qapi-schema.json,
> it's used to describe the dynamical dictionary/list/string.

s/dynamical/dynamic/

> 
> { 'type': 'DataObject',
>   'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> 
> Not all the keys in data will be used.
>  # List: type
>  # Dict: key, type
>  # nested List: type, data
>  # nested Dict: key, type, data

I wonder if we can take advantage of Kevin's work on unions to make this
MUCH easier to use:

https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html

{ 'enum': 'DataObjectType',
  'data': [ 'command', 'struct', 'union', 'enum' ] }
# extend to add 'event' later

{ 'type': 'DataObjectBase',
  'data': { 'name': 'str' } }

{ 'union': 'DataObjectMemberType',
  'discriminator': {},
  'data': { 'reference': 'str',
            'inline': 'DataObject' } }

{ 'type': DataObjectMember',
  'data': { 'name': 'str', 'type': 'DataObjectMemberType',
            '*optional': 'bool', '*recursive': 'bool' } }

{ 'type': 'DataObjectStruct',
  'data': { 'data': [ 'DataObjectMember' ] } }
{ 'type': 'DataObjectEnum',
  'data': { 'data': [ 'str' ] } }
{ 'type': 'DataObjectUnion',
  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
            '*discriminator': 'str' } }
{ 'type': 'DataObjectCommand',
  'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }

{ 'union': 'DataObject',
  'base': 'DataObjectBase',
  'discriminator': 'type',
  'data': {
    'struct': 'DataObjectStruct',
    'enum': 'DataObjectEnum',
    'union': 'DataObjectUnion',
    'command': 'DataObjectCommand',
    'array': {} }

> 
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
> 
> The following content gives an example of query-tpm-types:
> 
>  ## Define example in qapi-schema.json:
> 
>  { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
>  { 'command': 'query-tpm-types', 'returns': ['TpmType'] }

It might be more useful to have a (made-up) example that shows as many
details as possible - a command that takes arguments and has a return
type will exercise more of the code paths than a query command with only
a return.

> 
>  ## Returned description:
> 
>  {
>      "name": "query-tpm-types",
>      "type": "Command",
>      "returns": [
>          {
>              "type": "TpmType",
>              "data": [
>                  {
>                      "type": "passthrough"
>                  }
>              ]

I need a way to know the difference between a TpmType returned directly
in a dict, vs. a return containing an array of TpmType.

>          }
>      ]
>  },

Thus, using the discriminated union I set out above, I would return
something like:
{ "name": "TpmType", "type": "enum",
  "data": [ "passthrough" ] },
{ "name": "query-tpm-types", "type": "command",
  "data": [ ],
  "returns": { "name": "TpmType", "type": "array" } }

> 
> 'TpmType' is a defined type, it will be extended in returned
> description. [ 'passthrough' ] is a list, so 'type' and 'data'
> will be used.
> 
> TODO:
> We can add events definations to qapi-schema.json by another

s/definations/definitions/

> patch, then event can also be queried.
> 
> Introduce another command 'query-qga-schema' to query QGA schema
> information, it's easy to add this support with current current
> patch.

Such a command would be part of the QGA interface, not a QMP command.
But yes, it is needed, and the ideal patch series from you will include
that patch as part of the series.  But I don't mind waiting until we get
the interface nailed down before you actually implement the QGA repeat
of the interface.

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
>  qapi-schema.json                |  69 +++++++++
>  qmp-commands.hx                 |  39 +++++
>  qmp.c                           | 307 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 558 insertions(+)
>  create mode 100644 docs/qmp-full-introspection.txt
> 
> diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> new file mode 100644
> index 0000000..cc0fb80
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,143 @@
> += full introspection support for QMP =
> +

Is it worth merging this into the existing qapi-code-gen.txt, or at
least having the two documents refer to one another, since they deal
with related concepts (turning the .json file into generated code)?

> +== Purpose ==
> +
> +Add a new interface to provide QMP schema information to management,

s/a new/an/ - after a year, the interface won't be new, but this doc
will still be relevant.

> +the return data is a dynamical and nested dict/list, it contains

s/dynamical/dynamic/

> +the useful metadata to help management to check feature support,
> +QMP commands detail, etc.
> +
> +== Usage ==
> +
> +Execute QMP command:
> +
> +  { "execute": "query-qmp-schema" }
> +
> +Returns:
> +
> +  { "return": [
> +      {
> +          "name": "query-name",
> +          "type": "Command",
> +          "returns": [
> +              {
> +                  "key": "*name",
> +                  "type": "str"
> +              }
> +          ]

Are we trying to use struct names where they exist for compactness, or
trying to inline-expand everything as far as possible until we run into
self-referential recursion?

> +      },
> +      ...
> +   }
> +
> +The whole schema information will be returned in one go, it contains
> +commands and event. It doesn't support to be filtered by type or name.

s/event/events/
s/It/At present, it/
s/to be filtered/filtering/

> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)

This list will get out of date quickly.  I'd just word it generically:

We have several self-referential types

> +that uses themself in their own define data directly or indirectly,

s/uses themself/use themselves/

> +we will not repeatedly extend them to avoid dead loop.

Would it be worth a flag in the QMP output when a type is not being
further expanded because it is a nested occurrence of itself?  That is,
given my proposed layout above, ImageInfo would turn into something like:

{ "name": "ImageInfo", "type": "struct",
  "data": [ { "name": "filename", "type", "str" },
            ...
            { "name": "backing-image", "type": "ImageInfo",
               "optional": true, "recursive": true } ] },

> +
> +== more description of 'DataObject' type ==
> +
> +We use 'DataObject' to describe dynamical data struct, it might be

s/dynamical/dynamic/

> +nested dictionary, list or string.
> +
> +'DataObject' itself is a arbitrary and nested dictionary, the
> +dictionary has three keys ('key', 'type', 'data'), 'key' and

spacing is odd.

> +'data' are optional.
> +
> +* For describing Dictionary, we set the key to 'key', and set the
> +  value to 'type'
> +* For describing List, we don't set 'key', just set the value to
> +  'type'

Again, if you use the idea of a discriminated union, you don't need
quite the complexity in describing this: dictionaries are listed with
key/type/optional tuples, lists (enums) are listed with just an array of
strings, and the QAPI perfectly described that difference by the
discriminator telling you 'struct' vs. 'enum'.

> +* If the value of dictionary or list is non-native type, we extend
> +  the non-native type to dictionary, set it to 'data',  and set the
> +  non-native type's name to 'type'.

Again, I tried to set up the QAPI that describes something that uses an
anonymous union that either gives a string (the name of a primitive or
already-defined type) or a struct (a recursion into another layer of
struct describing the structure of that type in line).

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7b9fef1..cf03391 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3679,3 +3679,72 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +##
> +# @DataObject
> +#
> +# Details of a data object, it can be nested dictionary/list
> +#
> +# @key: #optional Data object key
> +#
> +# @type: Data object type name
> +#
> +# @optional: #optional whether the data object is optional

mention that the default is false.

> +#
> +# @data: #optional DataObject list, can be a dictionary or list type

so if 'data' is present, we are describing @type in more detail; if it
is absent, then @type is primitive.

> +#
> +# Since: 1.6
> +##
> +{ 'type': 'DataObject',
> +  'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
> +

'*type' should be an enum type, not open-coded string.


> +##
> +# @SchemaMetaType
> +#
> +# Possible meta types of a schema entry
> +#
> +# @Command: QMP command
> +#
> +# @Type: defined new data type
> +#
> +# @Enumeration: enumeration data type
> +#
> +# @Union: union data type
> +#
> +# Since: 1.6
> +##
> +{ 'enum': 'SchemaMetaType',
> +  'data': ['Command', 'Type', 'Enumeration', 'Union'] }

Do we need to start these with a capital?  On the other hand, having
them as capitals may make it easier to ensure we are outputting correct
information.
> +
> +##
> +# @SchemaEntry
> +#
> +# Details of schema items
> +#
> +# @type: Entry's type in string format
> +#
> +# @name: Entry name
> +#
> +# @data: #optional list of DataObject. This can have different meaning
> +#        depending on the 'type' value. For example, for a QMP command,
> +#        this member contains an argument listing. For an enumeration,
> +#        it contains the enum's values and so on

This argues for making it a union type.

> +#
> +# @returns: #optional list of DataObject, return data after executing
> +#           QMP command
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
> +  'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.

If you don't take any arguments, then the "returns an error" statement
is impossible.

> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e075df4..e3cbe93 100644
> --- a/qmp-commands.hx
>  
> +/*
> + * Use a string to record the visit path, type index of each node
> + * will be saved to the string, indexes are split by ':'.
> + */
> +static char visit_path_str[1024];

Is a fixed width buffer really the best solution, or does glib offer us
something better?  For example, a hash table, or even a growable string.

> +
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        assert(data != NULL);
> +
> +        qdict = qobject_to_qdict(data);
> +        assert(qdict != NULL);
> +
> +        ent = qdict_first(qdict);
> +        if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
> +            && !qdict_get(qdict, "union")) {
> +            continue;
> +        }

Why are we doing the work in C code, every time the command is called?
Wouldn't it be more efficient to do the work in python code, at the time
the qmp_schema_table C code is first generated, so that the generated
code is already in the right format?

> +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> +{
> +    SchemaEntryList *list, *last_entry, *entry;
> +    SchemaEntry *info;
> +    DataObjectList *obj_entry;
> +    DataObject *obj_info;
> +    QObject *data;
> +    QDict *qdict;
> +    int i;
> +
> +    list = NULL;
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        assert(data != NULL);
> +
> +        qdict = qobject_to_qdict(data);
> +        assert(qdict != NULL);
> +
> +        if (qdict_get(qdict, "command")) {
> +            info = g_malloc0(sizeof(*info));
> +            info->type = SCHEMA_META_TYPE_COMMAND;
> +            info->name = strdup(qdict_get_str(qdict, "command"));
> +        } else {
> +            continue;
> +        }
> +

Don't we want to also output types (struct, union, enum) and eventually
events, not just commands?

I hope we're converging, but I'm starting to worry that we won't have a
design in place in time for the 1.6 release.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]