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' ] }

It's necessary!!

> # 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' } }
 
Defining DataObject to a union, it will solve some hidden problem :)

> { '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.

OK.

> > 
> >  ## 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.

QObject is always a dictionary, it can describe list and dictionary.
I will added a 'KIND' field for QObject.
 
> >          }
> >      ]
> >  },
> 
> 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)?

This doc is not just about code generating (we just generated a simple
head file), most of work is about "full-introspection", the usage of
DataObject, implementation detail(visit path), examples.
 
> > +== 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?

inline-expand everything

> > +      },
> > +      ...
> > +   }
> > +
> > +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?

ok.

>  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.
 
Right!
 
> > +##
> > +# @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.

When we execute the full introspecting, we will parse the json strings
in the head file (converted from .json file). Return error if the
json string is invild in head file.
 
> > +#
> > +# 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.
 
1024 is enough by experience, and I have a check in push_id() which
will fill string in visit_path_str.

+static void push_id(int id)
+{
+    char *end = strrchr(visit_path_str, ':');
+    char type_idx[256];
+    int num;
+
+    num = sprintf(type_idx, "%d:", id);
+
+    if (end) {
+        /* avoid overflow */
+        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));


> > +
> > +    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?

Right now, we parse the json string, and generate a dynamic data
struct and set it as the output of QMP.

Parse json string by qapi (python) script and generate C code ( a big
tree, it's buildup by many nested structs. We need to visit the big
static tree and generate another dynamic tree for QMP.

I think current implement is better. I will try the python solution.
 
> > +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 think struct, union, enum are used in commands, so I didn't
repeatedly output them. If it's not repeated, I will output all of
them in next version.
 
> 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.


-- 
                        Amos.