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

[Amos Kong]

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 <address@hidden>
> > ---
> >  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
>

pgppARUhZXUR6.pgp
Description: PGP signature