Re: [Qemu-devel] [PATCH v4 4/5] qmp: full introspection support for QMP

[Amos Kong]

On Mon, Jan 27, 2014 at 10:38:24AM +0100, Paolo Bonzini wrote:
> Il 27/01/2014 09:17, Amos Kong ha scritto:
> >CC Libvirt-list
> >
> >Original discussion:
> >  http://marc.info/?l=qemu-devel&m=139048842504757&w=2
> >  [Qemu-devel] [PATCH v4 0/5] QMP full introspection
> >
> >On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> >>On Thu, 01/23 22:46, Amos Kong wrote:
> >>>This patch introduces a new monitor command to query QMP schema
> >>>information, the return data is a range of schema structs, which
> >>>contains the useful metadata to help management to check supported
> >>>features, QMP commands detail, etc.
> >>>
> >>>We use qapi-introspect.py to parse all json definition in
> >>>qapi-schema.json, and generate a range of dictionaries with metadata.
> >>>The query command will visit the dictionaries and fill the data
> >>>to allocated struct tree. Then QMP infrastructure will convert
> >>>the tree to json string and return to QMP client.
> >>>
> >>>TODO:
> >>>Wenchao Xia is working to convert QMP events to qapi-schema.json,
> >>>then event can also be queried by this interface.
> >>>
> >>>I will introduce another command 'query-qga-schema' to query QGA
> >>>schema information, it's easy to add this support based on this
> >>>patch.
> >>>
> >>>Signed-off-by: Amos Kong <address@hidden>
> >>>---
> >>> qapi-schema.json |  11 +++
> >>> qmp-commands.hx  |  42 +++++++++++
> >>> qmp.c            | 215 
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 268 insertions(+)
> >>>
> >>>diff --git a/qapi-schema.json b/qapi-schema.json
> >>>index c63f0ca..6033383 100644
> >>>--- a/qapi-schema.json
> >>>+++ b/qapi-schema.json
> >>>@@ -4411,3 +4411,14 @@
> >>>     'reference-type': 'String',
> >>>     'type': 'DataObjectType',
> >>>     'unionobj': 'DataObjectUnion' } }
> >>>+
> >>>+##
> >>>+# @query-qmp-schema
> >>>+#
> >>>+# Query QMP schema information
> >>>+#
> >>>+# @returns: list of @DataObject
> >>>+#
> >>>+# Since: 1.8
> >>>+##
> >>>+{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> >>>diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>>index 02cc815..b83762d 100644
> >>>--- a/qmp-commands.hx
> >>>+++ b/qmp-commands.hx
> >>>@@ -3291,6 +3291,48 @@ Example:
> >>>    }
> >>>
> >>> EQMP
> >>>+    {
> >>>+        .name       = "query-qmp-schema",
> >>>+        .args_type  = "",
> >>>+        .mhandler.cmd_new = qmp_marshal_input_query_qmp_schema,
> >>>+    },
> >>>+
> >>>+
> >>>+SQMP
> >>>+query-qmp-schema
> >>>+----------------
> >>>+
> >>>+query qmp schema information
> >>>+
> >>>+Return a json-object with the following information:
> >>>+
> >>>+- "name": qmp schema name (json-string)
> >>>+- "type": qmp schema type, it can be 'comand', 'type', 'enum', 'union'
> >>>+- "returns": return data of qmp command (json-object, optional)
> >>>+
> >>>+Example:
> >>>+
> >>>+-> { "execute": "query-qmp-schema" }
> >>>+-> { "return": [
> >>>+     {
> >>>+         "name": "query-name",
> >>>+         "type": "command",
> >>>+         "returns": {
> >>>+             "name": "NameInfo",
> >>>+             "type": "type",
> >>>+             "data": [
> >>>+                 {
> >>>+                     "name": "name",
> >>>+                     "optional": true,
> >>>+                     "recursive": false,
> >>>+                     "type": "str"
> >>>+                 }
> >>>+             ]
> >>>+         }
> >>>+     }
> >>>+  }
> >>>+
> >>>+EQMP
> >>>
> >>>     {
> >>>         .name       = "blockdev-add",
> >>>diff --git a/qmp.c b/qmp.c
> >>>index 0f46171..a64ae6d 100644
> >>>--- a/qmp.c
> >>>+++ b/qmp.c
> >>>@@ -27,6 +27,8 @@
> >>> #include "qapi/qmp/qobject.h"
> >>> #include "qapi/qmp-input-visitor.h"
> >>> #include "hw/boards.h"
> >>>+#include "qapi/qmp/qjson.h"
> >>>+#include "qapi-introspect.h"
> >>>
> >>> NameInfo *qmp_query_name(Error **errp)
> >>> {
> >>>@@ -488,6 +490,219 @@ CpuDefinitionInfoList 
> >>>*qmp_query_cpu_definitions(Error **errp)
> >>>     return arch_query_cpu_definitions(errp);
> >>> }
> >>>
> >>>+static strList *qobject_to_strlist(QObject *data)
> >>>+{
> >>>+    strList *list = NULL;
> >>>+    strList **plist = &list;
> >>>+    QList *qlist;
> >>>+    const QListEntry *lent;
> >>>+
> >>>+    qlist = qobject_to_qlist(data);
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        strList *entry = g_malloc0(sizeof(strList));
> >>>+        entry->value = g_strdup(qobject_get_str(lent->value));
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObject *qobject_to_dataobj(QObject *data);
> >>>+
> >>>+static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> >>>+{
> >>>+
> >>>+    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> >>>+
> >>>+    member->type = g_malloc0(sizeof(DataObjectMemberType));
> >>>+    if (data->type->code == QTYPE_QDICT) {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >>>+        member->type->extend = qobject_to_dataobj(data);
> >>>+    } else {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> >>>+        member->type->reference = g_strdup(qobject_get_str(data));
> >>>+    }
> >>>+
> >>>+    return member;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> >>>+{
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QDict *qdict = qobject_to_qdict(data);
> >>>+    const QDictEntry *dent;
> >>>+
> >>>+    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) 
> >>>{
> >>>+        DataObjectMemberList *entry = 
> >>>g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(dent->value);
> >>>+
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        if (dent->key[0] == '*') {
> >>>+            entry->value->optional = true;
> >>>+            entry->value->name = g_strdup(dent->key + 1);
> >>>+        } else {
> >>>+            entry->value->name = g_strdup(dent->key);
> >>>+        }
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> >>>+{
> >>>+    const QListEntry *lent;
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QList *qlist = qobject_to_qlist(data);
> >>>+
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        DataObjectMemberList *entry = 
> >>>g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(lent->value);
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_memlist(QObject *data)
> >>
> >>This whole converting is cumbersome. You already did all the traversing 
> >>through
> >>the type jungle in python when generating this, it's not necessary to do the
> >>similar thing again here.
> >
> >We can parse raw schemas and generate json string table, we can't
> >directly return the string / qobject to monitor, C code has to convert
> >the json to qobject, we have to revisit the qobject and convert them
> >to DataObject/DataObjectMember/DataObject... structs.
> >
> >>Alternatively, I think we have a good reason to extend QMP framework as
> >>necessary here, as we are doing "QMP introspection", which is a part of the
> >>framework:
> >>
> >> * Define final output into qmp_schema_table[], no need to box it like:
> >>
> >>        "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
> >>        'ErrorClass', '_obj_data': {'data': ...
> >>
> >>   just put it content of "qmp-introspection.output.txt" as a long string in
> >>   the header,
> >
> >
> >
> >>   like you would generate in qobject_to_memlist:
> >>
> >>        const char *qmp_schema_table =
> >>        "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
> >>        "{ 'name': '...', ...},"
> >
> >The keys are used for metadata might be 'recursive', 'optional', etc.
> >It might exists problem in namespace, let's use '_obj_' or '_' prefix
> >for the metadata keys.
> >
> >I used a nested dictionary to describe a DataObject, because we can
> >store the metadata and definition to different level, it's helpful
> >in parse the output by Libvirt.
> >
> > example:
> >   "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
> >
> >It's good to store _type, _name, data to same level, but the metadata
> >of items of data's value dictionary can't be appended to same level.
> >
> >    "{ '_name': 'NameInfo', '_type': 'type',
> >        'data': {
> >                  'name': 'str', '_name_optional': 'True',
> >                  'job': 'str', '_job_optional': 'True'
> >                }
> >     }"
> >
> >
> >A better solution, but I don't know if it will cause trouble for
> >Libvirt to parse the output.
> >
> >    "{'_type': 'type', '_name': 'NameInfo',
> >        'data': {  'job': {'_value': 'str', '_recursive': 'True'},
> >                   'name': {'_value': 'str', '_recursive': 'True'}
> >                },
> >        '_recursive': 'False'
> >     }"
> >
> >When we describe a DataObject (dict/list/str, one schema, extened
> >schema, schema member, etc), so I we generally use a nested dictionary
> >to describe a DataObject, it will split the metadata with original
> >data two different dictionary level, it's convenient for parse of
> >Libvirt. Here I just use a dict member as an example, actually
> >it's more complex to parse all kinds of data objects.
> >
> >>        ...
> >>        ;
> >>
> >> * Add a new type of qmp command, that returns a QString as a json literal.
> >>   query-qmp-schema is defined as this type. (This wouldn't be much code, 
> >> but
> >>   may be abused in the future, I'm afraid. However we can review, limit its
> >>   use to introspection only)
> >>
> >> * And return qmp_schema_table from query-qmp-shema, which will be copied to
> >>   the wire.
> >>
> >>Feel free to disagree, it's not a perfect solution. But I really think we 
> >>need
> >>to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
> >
> >
> >In the past, we didn't consider to extend and add metadata by Python, so
> >Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> >But now, we already successfully to transfer this work to Python, and
> >we can adjust to make management to be happy for the metadata and
> >format.
> >
> >The problem is that Libvirt should parse the output twice, the json
> >items are also json object string.
> >
> >Eric, Is it acceptabled?
> >
> >  Example:
> >  * as suggested by Fam to put the metadta with schema data in same
> >    dict level
> >  * process some special cases by nested dictionary
> >    (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
> >  * return a strList to Libvirt, the content of string is json object,
> >    that contains the metadata.
> >
> >{
> >    "return": [
> >        "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 
> > 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 
> > 'KVMMissingCap'], '_recursive': 'False'}",
> >        "{'_type': 'command', '_name': 'add_client', 'data': {'tls': 
> > {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', 
> > '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 
> > 'False'}",
> >        "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 
> > 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 
> > 'True'}}, '_recursive': 'False'}",
> >        "{'returns': {'_type': 'type', '_recursive': 'False', 'data': 
> > {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', 
> > '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', 
> > '_type': 'command', '_recursive': 'False'}",
> >        "......",
> >        "......",
> >        "......"
> >    ]
> >}
> >
> >>Fam
> >
> 
> No, I don't like this.
> 
> QAPI types are perfectly able to "describe themselves", there is no
> need to escape to JSON.
> 
> Let's just do what this series is doing, minus the unnecessary recursion.
 
The recursion extend was requested by Libvirt, they want to get a
extended output with metadata to avoid heavy parsing in Libvirt.

Let's see the response of libvirt people.

> Paolo

-- 
                        Amos.