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 <ak...@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
signature.asc
Description: OpenPGP digital signature
