On Fri, Jul 19, 2013 at 04:05:16PM -0600, Eric Blake wrote:
> 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': {} }
In my patch, I used a _dictionary_ to describe this kind of thing
1) dict, 2) list, 3) str
The above line is used for Dictionary or List, it should be:
'array': ['DataObject']
But I touched a new error:
qapi-visit.c: In function ‘visit_type_DataObject’:
qapi-visit.c:7255:29: error: implicit declaration of function
‘visit_type_DataObjectList_fields’ [-Werror=implicit-function-declaration]
visit_type_DataObjectList_fields(m,
&(*obj)->array, &err);
----
So I try to defined
{ 'type': 'DataObjectArray', 'data': ['DataObject'] }
'DataObjectArrayType' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'name',
'data': {
'array': 'DataObjectArray',
got error:
Traceback (most recent call last):
File "/home/devel/qemu/scripts/qapi-types.py", line 471, in <module>
ret += generate_struct(expr) + "\n"
File "/home/devel/qemu/scripts/qapi-types.py", line 101, in generate_struct
ret += generate_struct_fields(members)
File "/home/devel/qemu/scripts/qapi-types.py", line 67, in
generate_struct_fields
for argname, argentry, optional, structured in parse_args(members):
File "/home/devel/qemu/scripts/qapi.py", line 197, in parse_args
argentry = typeinfo[member]
TypeError: list indices must be integers, not str
----
a new definition:
{ 'enum': 'DataObjectArrayType', 'data': ['Dictionary', 'List'] }
{ 'type': 'DataObjectArray', 'data': {'data': ['DataObject'], 'type':
'DataObjectArrayType' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'name',
'data': {
'array': 'DataObjectArray',
-----
In my V2, I parse the schema just according the Format attribute (dict, str,
list)
Eric suggested defination is wonderful, but it's not flexible as mine ;-)
The data type, format (dict/str/list), more matadata should be considered.
It makes the parse very complex.
I have to simple it, the matadata will also provided, just make the
parse work easyer. Libvirt can still get good info as using Eric's
defination.
Thanks, Amos
> > 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
>
pgpI_nAcS7sqS.pgp
Description: PGP signature
