[Qemu-devel] [PATCH v4 0/5] QMP full introspection

[Qemu-devel] [PATCH v4 0/5] QMP full introspection

[Amos Kong]

This is an implement of qmp full-introspection,
parse and convert the json schema to a dynamical tree,
return it to management through QMP command output.

The whole output of query-qmp-schema command:
 http://i-kvm.rhcloud.com/static/pub/v4/qmp-introspection.output.txt
 http://i-kvm.rhcloud.com/static/pub/v4/qmp-introspection.h

Welcome your comments!

V2: use 'DataObject' to describe dynamic struct
V3: improve the metadata as suggested by eric
V4: use python to extend/parse schema for improving
    the response speed and simple the code 

Amos Kong (5):
  qapi: introduce DataObject to describe dynamic structs
  qapi: add qapi-introspect.py code generator
  qobject: introduce qobject_get_str()
  qmp: full introspection support for QMP
  update docs/qmp-full-introspection.txt

 .gitignore                      |   1 +
 Makefile                        |   5 +-
 docs/qmp-full-introspection.txt |  99 ++++++++++++++++++
 include/qapi/qmp/qstring.h      |   1 +
 qapi-schema.json                | 152 ++++++++++++++++++++++++++++
 qmp-commands.hx                 |  42 ++++++++
 qmp.c                           | 215 ++++++++++++++++++++++++++++++++++++++++
 qobject/qstring.c               |  19 ++++
 scripts/qapi-introspect.py      | 172 ++++++++++++++++++++++++++++++++
 9 files changed, 705 insertions(+), 1 deletion(-)
 create mode 100644 docs/qmp-full-introspection.txt
 create mode 100644 scripts/qapi-introspect.py

-- 
1.8.4.2

[Qemu-devel] [PATCH v4 1/5] qapi: introduce DataObject to describe dynamic structs

[Amos Kong]

This patch introduced a DataObject union in qapi-schema.json,
we use it to describe dynamic data structs.

We will use it in following patches to support to QMP full
introspection. We have many kinds of schema in json file,
they all can be described by DataObject.

This patch also added a doc: qmp-full-introspection.txt,
QMP introspection releated document will be added into it.
It helps to use the new query command and understand the
abstract method in describing the dynamic struct.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 docs/qmp-full-introspection.txt |  44 +++++++++++++
 qapi-schema.json                | 141 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 185 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..d2cf7b3
--- /dev/null
+++ b/docs/qmp-full-introspection.txt
@@ -0,0 +1,44 @@
+= Full introspection support for QMP =
+
+
+== Purpose ==
+
+Add a new monitor command for management to  query QMP schema
+information, it returns a range of schema structs, which contain the
+useful metadata to help management to check supported features, QMP
+commands detail, etc.
+
+== 'DataObject' union ==
+
+{ 'union': 'DataObject',
+  'base': 'DataObjectBase',
+  'discriminator': 'type',
+  'data': {
+    'anonymous-struct': 'DataObjectAnonymousStruct',
+    'command': 'DataObjectCommand',
+    'enumeration': 'DataObjectEnumeration',
+    'reference-type': 'String',
+    'type': 'DataObjectType',
+    'unionobj': 'DataObjectUnion' } }
+
+Currently we have schema difinitions for type, command, enumeration,
+union. Some arbitrary structs (dictionary, list or string) and native
+types are also used in the body of definitions.
+
+Here we use "DataObject" union to abstract all above schema. We want
+to provide more useful metadata, and used some enum/unions to indicate
+the dynamic type. In the output, some simple data is processed too
+unwieldy. In another side, some complex data is described clearly.
+It's also caused by some limitation of QAPI infrastructure.
+
+So we define 'DataObject' to be an union, it always has an object name
+except anonymous struct.
+
+'command', 'enumeration', 'type', 'unionobj' are common schema type,
+'union' is a build-in type, so I used unionobj here.
+
+'reference-type' will be used to describe native types and unextended
+types.
+
+'anonymous-struct' will be used to describe arbitrary structs
+(dictionary, list or string).
diff --git a/qapi-schema.json b/qapi-schema.json
index f27c48a..c63f0ca 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4270,3 +4270,144 @@
 # Since: 1.7
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @DataObjectBase
+#
+# Base attributes of @DataObject
+#
+# @name: #optional @DataObject name
+# @type: @DataObject type
+# @recursive: #optional key to indicate if it's extended
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectBase',
+  'data': { '*name': 'str', 'type': 'str', '*recursive': 'bool' } }
+
+##
+# @DataObjectMemberType
+#
+# Type of @DabaObjectMember
+#
+# @reference: reference string
+# @anonymous: arbitrary struct
+# @extend: the @DataObjectMember
+#
+# Since: 1.8
+##
+{ 'union': 'DataObjectMemberType',
+  'discriminator': {},
+  'data': { 'reference': 'str',
+            'anonymous': 'DataObject',
+            'extend': 'DataObject' } }
+
+##
+# @DataObjectMember
+#
+# General member of @DataObject
+#
+# @type: type of @DataObjectMember
+# @name: #optional name
+# @optional: #optional key to indicate if the @DataObjectMember is optional
+# @recursive: #optional key to indicate if it's defined recursively
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectMember',
+  'data': { 'type': 'DataObjectMemberType', '*name': 'str',
+            '*optional': 'bool', '*recursive': 'bool' } }
+
+##
+# @DataObjectAnonymousStruct
+#
+# Arbitrary struct, it can be dictionary, list or string
+#
+# @data: content of arbitrary struct
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectAnonymousStruct',
+  'data': { 'data': [ 'DataObjectMember' ] } }
+
+##
+# @DataObjectCommand
+#
+# QMP Command schema
+#
+# @data: QMP command content
+# @returns: returns of executing command
+# @gen: a key to suppress code generation
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectCommand',
+  'data': { '*data': [ 'DataObjectMember' ],
+            '*returns': 'DataObject',
+            '*gen': 'bool' } }
+
+##
+# @DataObjectEnumeration
+#
+# Enumeration schema
+#
+# @data: enumeration content, it's a string list
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectEnumeration',
+  'data': { 'data': [ 'str' ] } }
+
+##
+# @DataObjectType
+#
+# Type schema
+#
+# @data: defined content of type, it's a dictionary or list
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectType',
+  'data': { 'data': [ 'DataObjectMember' ] } }
+
+##
+# @DataObjectUnion
+#
+# Union schema
+#
+# @data: main content of union
+# @base: base attributes of union
+# @discriminator: union discriminator
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectUnion',
+  'data': { 'data': [ 'DataObjectMember' ], '*base': 'DataObject',
+            '*discriminator': 'DataObject' } }
+
+##
+# @DataObject
+#
+# Dynamic data struct, it can be command, enumeration, type, union, arbitrary
+# struct or native type.
+#
+# @anonymous-struct: arbitrary struct, it can be dictionary, list or string
+# @command: QMP command schema
+# @enumeration: enumeration schema
+# @reference-type: native type or unextended type
+# @type: type schema, it will be extended
+# @unionobj: union schema, 'union' is a conflicted name, so we use
+#            unionobj instead
+#
+# Since: 1.8
+##
+{ 'union': 'DataObject',
+  'base': 'DataObjectBase',
+  'discriminator': 'type',
+  'data': {
+    'anonymous-struct': 'DataObjectAnonymousStruct',
+    'command': 'DataObjectCommand',
+    'enumeration': 'DataObjectEnumeration',
+    'reference-type': 'String',
+    'type': 'DataObjectType',
+    'unionobj': 'DataObjectUnion' } }
-- 
1.8.4.2

[Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Amos Kong]

This is a code generator for qapi introspection. It will parse
qapi-schema.json, extend schema definitions and generate a schema
table with metadata, it references to the new structs which we used
to describe dynamic data structs.  The metadata will help C code to
allocate right structs and provide useful information to management
to checking suported feature and QMP commandline detail. The schema
table will be saved to qapi-introspect.h.

The $(prefix) is used to as a namespace to keep the generated code
from one schema/code-generation separated from others so code and
be generated from multiple schemas with clobbering previously
created code.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 .gitignore                      |   1 +
 Makefile                        |   5 +-
 docs/qmp-full-introspection.txt |  17 ++++
 scripts/qapi-introspect.py      | 172 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 194 insertions(+), 1 deletion(-)
 create mode 100644 scripts/qapi-introspect.py

diff --git a/.gitignore b/.gitignore
index 1c9d63d..de3cb80 100644
--- a/.gitignore
+++ b/.gitignore
@@ -22,6 +22,7 @@ linux-headers/asm
 qapi-generated
 qapi-types.[ch]
 qapi-visit.[ch]
+qapi-introspect.h
 qmp-commands.h
 qmp-marshal.c
 qemu-doc.html
diff --git a/Makefile b/Makefile
index bdff4e4..1dac5e7 100644
--- a/Makefile
+++ b/Makefile
@@ -45,7 +45,7 @@ endif
 endif
 
 GENERATED_HEADERS = config-host.h qemu-options.def
-GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
+GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-introspect.h
 GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c
 
 GENERATED_HEADERS += trace/generated-events.h
@@ -229,6 +229,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
 qmp-commands.h qmp-marshal.c :\
 $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
+qapi-introspect.h:\
+$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py $(gen-out-type) -o "." < $<, "  GEN   $@")
 
 QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
 $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
index d2cf7b3..8ecbc0c 100644
--- a/docs/qmp-full-introspection.txt
+++ b/docs/qmp-full-introspection.txt
@@ -42,3 +42,20 @@ types.
 
 'anonymous-struct' will be used to describe arbitrary structs
 (dictionary, list or string).
+
+== Avoid dead loop in recursive extending ==
+
+We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
+that uses themself in their own define data directly or indirectly,
+we will not repeatedly extend them to avoid dead loop.
+
+We use a 'parents List' to record the visit path, type name of each
+extended node will be saved to the List.
+
+Append type name to the list before extending, and remove type name
+from the list after extending.
+
+If the type name is already extended in parents List, we won't extend
+it repeatedly for avoiding dead loop.
+
+'recursive' indicates if the type is extended or not.
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
new file mode 100644
index 0000000..03179fa
--- /dev/null
+++ b/scripts/qapi-introspect.py
@@ -0,0 +1,172 @@
+#
+# QAPI introspection info generator
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# Authors:
+#  Amos Kong <akong@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPLv2.
+# See the COPYING.LIB file in the top-level directory.
+
+from ordereddict import OrderedDict
+from qapi import *
+import sys
+import os
+import getopt
+import errno
+
+
+try:
+    opts, args = getopt.gnu_getopt(sys.argv[1:], "hp:o:",
+                                   ["header", "prefix=", "output-dir="])
+except getopt.GetoptError, err:
+    print str(err)
+    sys.exit(1)
+
+output_dir = ""
+prefix = ""
+h_file = 'qapi-introspect.h'
+
+do_h = False
+
+for o, a in opts:
+    if o in ("-p", "--prefix"):
+        prefix = a
+    elif o in ("-o", "--output-dir"):
+        output_dir = a + "/"
+    elif o in ("-h", "--header"):
+        do_h = True
+
+h_file = output_dir + prefix + h_file
+
+try:
+    os.makedirs(output_dir)
+except os.error, e:
+    if e.errno != errno.EEXIST:
+        raise
+
+def maybe_open(really, name, opt):
+    if really:
+        return open(name, opt)
+    else:
+        import StringIO
+        return StringIO.StringIO()
+
+fdecl = maybe_open(do_h, h_file, 'w')
+
+fdecl.write(mcgen('''
+/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+
+/*
+ * Head file to store parsed information of QAPI schema
+ *
+ * Copyright (C) 2014 Red Hat, Inc.
+ *
+ * Authors:
+ *  Amos Kong <akong@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef %(guard)s
+#define %(guard)s
+
+''',
+                  guard=guardname(h_file)))
+
+def extend_schema(expr, parents=[], member=True):
+    ret = {}
+    recu = 'False'
+    name = ""
+
+    if type(expr) is OrderedDict:
+        if not member:
+            e = expr.popitem(last=False)
+            typ = e[0]
+            name = e[1]
+        else:
+            typ = "anonymous-struct"
+
+        if typ == 'enum':
+            for key in expr.keys():
+                ret[key] = expr[key]
+        else:
+            ret = {}
+            for key in expr.keys():
+                ret[key], parents = extend_schema(expr[key], parents)
+
+    elif type(expr) is list:
+        typ = 'anonymous-struct'
+        ret = []
+        for i in expr:
+            tmp, parents = extend_schema(i, parents)
+            ret.append(tmp)
+    elif type(expr) is str:
+        name = expr
+        if schema_dict.has_key(expr) and expr not in parents:
+            parents.append(expr)
+            typ = schema_dict[expr][1]
+            recu = 'True'
+            ret, parents = extend_schema(schema_dict[expr][0].copy(),
+                                         parents, False)
+            parents.remove(expr)
+            ret['_obj_recursive'] = 'True'
+            return ret, parents
+        else:
+            return expr, parents
+
+    return {'_obj_member': "%s" % member, '_obj_type': typ,
+            '_obj_name': name, '_obj_recursive': recu,
+            '_obj_data': ret}, parents
+
+
+exprs = parse_schema(sys.stdin)
+schema_dict = {}
+
+for expr in exprs:
+    if expr.has_key('type') or expr.has_key('enum') or expr.has_key('union'):
+        e = expr.copy()
+
+        first = e.popitem(last=False)
+        schema_dict[first[1]] = [expr.copy(), first[0]]
+
+fdecl.write('''const char *const qmp_schema_table[] = {
+''')
+
+def convert(odict):
+    d = {}
+    for k, v in odict.items():
+        if type(v) is OrderedDict:
+            d[k] = convert(v)
+        elif type(v) is list:
+            l = []
+            for j in v:
+                if type(j) is OrderedDict:
+                    l.append(convert(j))
+                else:
+                    l.append(j)
+            d[k] = l
+        else:
+            d[k] = v
+    return d
+
+count = 0
+for expr in exprs:
+    fdecl.write('''    /* %s */
+''' % expr)
+
+    expr, parents = extend_schema(expr, [], False)
+    fdecl.write('''    "%s",
+
+''' % convert(expr))
+
+fdecl.write('''    NULL };
+
+#endif
+''')
+
+fdecl.flush()
+fdecl.close()
-- 
1.8.4.2

[Qemu-devel] [PATCH v4 3/5] qobject: introduce qobject_get_str()

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 include/qapi/qmp/qstring.h |  1 +
 qobject/qstring.c          | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 1bc3666..56b17cb 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -28,6 +28,7 @@ QString *qstring_from_str(const char *str);
 QString *qstring_from_substr(const char *str, int start, int end);
 size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
+const char *qobject_get_str(const QObject *obj);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 607b7a1..c470a86 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -135,6 +135,25 @@ const char *qstring_get_str(const QString *qstring)
 }
 
 /**
+ * qobject_to_str(): Convert a QObject to QString and return
+ * a pointer to the stored string
+ */
+const char *qobject_get_str(const QObject *data)
+{
+    QString *qstr;
+
+    if (!data) {
+        return NULL;
+    }
+    qstr = qobject_to_qstring(data);
+    if (qstr) {
+        return qstring_get_str(qstr);
+    } else {
+        return NULL;
+    }
+}
+
+/**
  * qstring_destroy_obj(): Free all memory allocated by a QString
  * object
  */
-- 
1.8.4.2

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

[Amos Kong]

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 <akong@redhat.com>
---
 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)
+{
+    DataObjectMemberList *list = NULL;
+    QDict *qdict = qobject_to_qdict(data);
+    QObject *subdata = qdict_get(qdict, "_obj_data");
+
+    list = NULL;
+    if (subdata->type->code == QTYPE_QDICT) {
+        list = qobject_to_dict_memlist(subdata);
+    } else if (subdata->type->code == QTYPE_QLIST) {
+        list = qobject_to_list_memlist(subdata);
+    }
+
+    return list;
+}
+
+static DataObject *qobject_to_dataobj(QObject *data)
+{
+    QObject *subdata;
+    QDict *qdict;
+    const char *obj_type, *obj_recursive;
+    DataObject *obj = g_malloc0(sizeof(DataObject));
+
+    if (data->type->code == QTYPE_QSTRING) {
+        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
+        obj->reference_type = g_malloc0(sizeof(String));
+        obj->reference_type->str = g_strdup(qobject_get_str(data));
+        return obj;
+    }
+
+    qdict = qobject_to_qdict(data);
+    assert(qdict != NULL);
+
+    obj_type = qobject_get_str(qdict_get(qdict, "_obj_type"));
+    obj_recursive = qobject_get_str(qdict_get(qdict, "_obj_recursive"));
+    if (!strcmp(obj_recursive, "True")) {
+        obj->has_recursive = true;
+        obj->recursive = true;
+    }
+
+    obj->has_name = true;
+    obj->name = g_strdup(qobject_get_str(qdict_get(qdict, "_obj_name")));
+
+    subdata = qdict_get(qdict, "_obj_data");
+    qdict = qobject_to_qdict(subdata);
+
+    if (!strcmp(obj_type, "command")) {
+        obj->kind = DATA_OBJECT_KIND_COMMAND;
+        obj->command = g_malloc0(sizeof(DataObjectCommand));
+        subdata = qdict_get(qobject_to_qdict(subdata), "data");
+
+        if (subdata && subdata->type->code == QTYPE_QDICT) {
+            obj->command->has_data = true;
+            obj->command->data = qobject_to_memlist(subdata);
+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
+            abort();
+        }
+
+        subdata = qdict_get(qdict, "returns");
+        if (subdata) {
+            obj->command->has_returns = true;
+            obj->command->returns = qobject_to_dataobj(subdata);
+        }
+
+        subdata = qdict_get(qdict, "gen");
+        if (subdata && subdata->type->code == QTYPE_QSTRING) {
+            obj->command->has_gen = true;
+            if (!strcmp(qobject_get_str(subdata), "no")) {
+                obj->command->gen = false;
+            } else {
+                obj->command->gen = true;
+            }
+        }
+    } else if (!strcmp(obj_type, "union")) {
+        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
+        obj->unionobj = g_malloc0(sizeof(DataObjectUnion));
+        subdata = qdict_get(qdict, "data");
+        obj->unionobj->data = qobject_to_memlist(subdata);
+
+        subdata = qdict_get(qdict, "base");
+        if (subdata) {
+            obj->unionobj->has_base = true;
+            obj->unionobj->base = qobject_to_dataobj(subdata);
+        }
+
+        subdata = qdict_get(qdict, "discriminator");
+        if (subdata) {
+            obj->unionobj->has_discriminator = true;
+            obj->unionobj->discriminator = qobject_to_dataobj(subdata);
+        }
+    } else if (!strcmp(obj_type, "type")) {
+        obj->kind = DATA_OBJECT_KIND_TYPE;
+        obj->type = g_malloc0(sizeof(DataObjectType));
+        subdata = qdict_get(qdict, "data");
+        if (subdata) {
+            obj->type->data = qobject_to_memlist(subdata);
+        }
+     } else if (!strcmp(obj_type, "enum")) {
+        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
+        obj->enumeration = g_malloc0(sizeof(DataObjectEnumeration));
+        subdata = qdict_get(qdict, "data");
+        obj->enumeration->data = qobject_to_strlist(subdata);
+    } else {
+        obj->has_name = false;
+        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
+        obj->anonymous_struct = g_malloc0(sizeof(DataObjectAnonymousStruct));
+        obj->anonymous_struct->data = qobject_to_memlist(data);
+    }
+
+    return obj;
+}
+
+DataObjectList *qmp_query_qmp_schema(Error **errp)
+{
+    DataObjectList *list = NULL;
+    DataObjectList **plist = &list;
+    QObject *data;
+    int i;
+
+    for (i = 0; qmp_schema_table[i]; i++) {
+        data = qobject_from_json(qmp_schema_table[i]);
+        assert(data != NULL);
+        DataObjectList *entry = g_malloc0(sizeof(DataObjectList));
+        entry->value = qobject_to_dataobj(data);
+        *plist = entry;
+        plist = &entry->next;
+    }
+
+    return list;
+}
+
 void qmp_add_client(const char *protocol, const char *fdname,
                     bool has_skipauth, bool skipauth, bool has_tls, bool tls,
                     Error **errp)
-- 
1.8.4.2

[Qemu-devel] [PATCH v4 5/5] update docs/qmp-full-introspection.txt

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 docs/qmp-full-introspection.txt | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
index 8ecbc0c..4cb1b9e 100644
--- a/docs/qmp-full-introspection.txt
+++ b/docs/qmp-full-introspection.txt
@@ -8,6 +8,44 @@ information, it returns a range of schema structs, which contain the
 useful metadata to help management to check supported features, QMP
 commands detail, etc.
 
+== Usage ==
+
+Json schema:
+  { 'type': 'NameInfo', 'data': {'*name': 'str'} }
+  { 'command': 'query-name', 'returns': 'NameInfo' }
+
+Execute QMP command:
+
+  { "execute": "query-qmp-schema" }
+
+Returns:
+
+  { "return": [
+      {
+          "name": "query-name",
+          "type": "command",
+          "returns": {
+              "name": "NameInfo",
+              "type": "type",
+              "data": [
+                  {
+                      "name": "name",
+                      "optional": true,
+                      "recursive": false,
+                      "type": "str"
+                  }
+              ]
+          }
+      },
+      ...
+   }
+
+The whole schema information will be returned in one go, it contains
+all the schema entries. It doesn't support to be filtered by type
+or name. Currently it takes about 4 seconds to return about 1.7M string.
+Management only needs to execute this command once after installing
+QEMU package.
+
 == 'DataObject' union ==
 
 { 'union': 'DataObject',
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v4 0/5] QMP full introspection

[Amos Kong]

On Thu, Jan 23, 2014 at 10:46:31PM +0800, Amos Kong wrote:
> This is an implement of qmp full-introspection,
> parse and convert the json schema to a dynamical tree,
> return it to management through QMP command output.
> 

Correct the URLs

> The whole output of query-qmp-schema command:
>  http://i-kvm.rhcloud.com/static/pub/v4/qmp-introspection.output.txt
>  http://i-kvm.rhcloud.com/static/pub/v4/qmp-introspection.h
 
https://i-kvm.rhcloud.com/static/pub/v4/qmp-introspection.output.txt
https://i-kvm.rhcloud.com/static/pub/v4/qapi-introspect.h

> 
> Welcome your comments!
> 
> V2: use 'DataObject' to describe dynamic struct
> V3: improve the metadata as suggested by eric
> V4: use python to extend/parse schema for improving
>     the response speed and simple the code 
> 
> Amos Kong (5):
>   qapi: introduce DataObject to describe dynamic structs
>   qapi: add qapi-introspect.py code generator
>   qobject: introduce qobject_get_str()
>   qmp: full introspection support for QMP
>   update docs/qmp-full-introspection.txt

Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Fam Zheng]

On Thu, 01/23 22:46, Amos Kong wrote:
> This is a code generator for qapi introspection. It will parse
> qapi-schema.json, extend schema definitions and generate a schema
> table with metadata, it references to the new structs which we used
> to describe dynamic data structs.  The metadata will help C code to
> allocate right structs and provide useful information to management
> to checking suported feature and QMP commandline detail. The schema
> table will be saved to qapi-introspect.h.
> 
> The $(prefix) is used to as a namespace to keep the generated code

s/used to as/used as/

> from one schema/code-generation separated from others so code and
> be generated from multiple schemas with clobbering previously

s/with/without/

> created code.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  .gitignore                      |   1 +
>  Makefile                        |   5 +-
>  docs/qmp-full-introspection.txt |  17 ++++
>  scripts/qapi-introspect.py      | 172 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 194 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/qapi-introspect.py
> 
> diff --git a/.gitignore b/.gitignore
> index 1c9d63d..de3cb80 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -22,6 +22,7 @@ linux-headers/asm
>  qapi-generated
>  qapi-types.[ch]
>  qapi-visit.[ch]
> +qapi-introspect.h
>  qmp-commands.h
>  qmp-marshal.c
>  qemu-doc.html
> diff --git a/Makefile b/Makefile
> index bdff4e4..1dac5e7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -45,7 +45,7 @@ endif
>  endif
>  
>  GENERATED_HEADERS = config-host.h qemu-options.def
> -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-introspect.h
>  GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c
>  
>  GENERATED_HEADERS += trace/generated-events.h
> @@ -229,6 +229,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
>  qmp-commands.h qmp-marshal.c :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> +qapi-introspect.h:\
> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py $(gen-out-type) -o "." < $<, "  GEN   $@")
>  
>  QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
>  $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
> diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> index d2cf7b3..8ecbc0c 100644
> --- a/docs/qmp-full-introspection.txt
> +++ b/docs/qmp-full-introspection.txt
> @@ -42,3 +42,20 @@ types.
>  
>  'anonymous-struct' will be used to describe arbitrary structs
>  (dictionary, list or string).
> +
> +== Avoid dead loop in recursive extending ==
> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
> +that uses themself in their own define data directly or indirectly,

s/themself/themselves/
s/define data/definition/

> +we will not repeatedly extend them to avoid dead loop.
> +
> +We use a 'parents List' to record the visit path, type name of each
> +extended node will be saved to the List.
> +
> +Append type name to the list before extending, and remove type name
> +from the list after extending.
> +
> +If the type name is already extended in parents List, we won't extend
> +it repeatedly for avoiding dead loop.

This "parents" list detail is not reflected in the generated information,
right?  I think it's good enough to describe that "type will not be extented
more than once in a schema, when there's direct or indirect recursive
type composition".

> +
> +'recursive' indicates if the type is extended or not.
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> new file mode 100644
> index 0000000..03179fa
> --- /dev/null
> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,172 @@
> +#
> +# QAPI introspection info generator
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# Authors:
> +#  Amos Kong <akong@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPLv2.
> +# See the COPYING.LIB file in the top-level directory.
> +
> +from ordereddict import OrderedDict
> +from qapi import *
> +import sys
> +import os
> +import getopt
> +import errno
> +
> +
> +try:
> +    opts, args = getopt.gnu_getopt(sys.argv[1:], "hp:o:",
> +                                   ["header", "prefix=", "output-dir="])
> +except getopt.GetoptError, err:
> +    print str(err)
> +    sys.exit(1)
> +
> +output_dir = ""
> +prefix = ""
> +h_file = 'qapi-introspect.h'
> +
> +do_h = False
> +
> +for o, a in opts:
> +    if o in ("-p", "--prefix"):
> +        prefix = a

Is this option used in your series?

Thanks,
Fam

> +    elif o in ("-o", "--output-dir"):
> +        output_dir = a + "/"
> +    elif o in ("-h", "--header"):
> +        do_h = True
> +
> +h_file = output_dir + prefix + h_file
> +
> +try:
> +    os.makedirs(output_dir)
> +except os.error, e:
> +    if e.errno != errno.EEXIST:
> +        raise
> +
> +def maybe_open(really, name, opt):
> +    if really:
> +        return open(name, opt)
> +    else:
> +        import StringIO
> +        return StringIO.StringIO()
> +
> +fdecl = maybe_open(do_h, h_file, 'w')
> +
> +fdecl.write(mcgen('''
> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Head file to store parsed information of QAPI schema
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * Authors:
> + *  Amos Kong <akong@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef %(guard)s
> +#define %(guard)s
> +
> +''',
> +                  guard=guardname(h_file)))
> +
> +def extend_schema(expr, parents=[], member=True):
> +    ret = {}
> +    recu = 'False'
> +    name = ""
> +
> +    if type(expr) is OrderedDict:
> +        if not member:
> +            e = expr.popitem(last=False)
> +            typ = e[0]
> +            name = e[1]
> +        else:
> +            typ = "anonymous-struct"
> +
> +        if typ == 'enum':
> +            for key in expr.keys():
> +                ret[key] = expr[key]
> +        else:
> +            ret = {}
> +            for key in expr.keys():
> +                ret[key], parents = extend_schema(expr[key], parents)
> +
> +    elif type(expr) is list:
> +        typ = 'anonymous-struct'
> +        ret = []
> +        for i in expr:
> +            tmp, parents = extend_schema(i, parents)
> +            ret.append(tmp)
> +    elif type(expr) is str:
> +        name = expr
> +        if schema_dict.has_key(expr) and expr not in parents:
> +            parents.append(expr)
> +            typ = schema_dict[expr][1]
> +            recu = 'True'
> +            ret, parents = extend_schema(schema_dict[expr][0].copy(),
> +                                         parents, False)
> +            parents.remove(expr)
> +            ret['_obj_recursive'] = 'True'
> +            return ret, parents
> +        else:
> +            return expr, parents
> +
> +    return {'_obj_member': "%s" % member, '_obj_type': typ,
> +            '_obj_name': name, '_obj_recursive': recu,
> +            '_obj_data': ret}, parents
> +
> +
> +exprs = parse_schema(sys.stdin)
> +schema_dict = {}
> +
> +for expr in exprs:
> +    if expr.has_key('type') or expr.has_key('enum') or expr.has_key('union'):
> +        e = expr.copy()
> +
> +        first = e.popitem(last=False)
> +        schema_dict[first[1]] = [expr.copy(), first[0]]
> +
> +fdecl.write('''const char *const qmp_schema_table[] = {
> +''')
> +
> +def convert(odict):
> +    d = {}
> +    for k, v in odict.items():
> +        if type(v) is OrderedDict:
> +            d[k] = convert(v)
> +        elif type(v) is list:
> +            l = []
> +            for j in v:
> +                if type(j) is OrderedDict:
> +                    l.append(convert(j))
> +                else:
> +                    l.append(j)
> +            d[k] = l
> +        else:
> +            d[k] = v
> +    return d
> +
> +count = 0
> +for expr in exprs:
> +    fdecl.write('''    /* %s */
> +''' % expr)
> +
> +    expr, parents = extend_schema(expr, [], False)
> +    fdecl.write('''    "%s",
> +
> +''' % convert(expr))
> +
> +fdecl.write('''    NULL };
> +
> +#endif
> +''')
> +
> +fdecl.flush()
> +fdecl.close()
> -- 
> 1.8.4.2
> 
> 

Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Amos Kong]

On Fri, Jan 24, 2014 at 05:12:12PM +0800, Fam Zheng wrote:
> On Thu, 01/23 22:46, Amos Kong wrote:
> > This is a code generator for qapi introspection. It will parse
> > qapi-schema.json, extend schema definitions and generate a schema
> > table with metadata, it references to the new structs which we used
> > to describe dynamic data structs.  The metadata will help C code to
> > allocate right structs and provide useful information to management
> > to checking suported feature and QMP commandline detail. The schema
> > table will be saved to qapi-introspect.h.
> > 
> > The $(prefix) is used to as a namespace to keep the generated code
> 
> s/used to as/used as/
> 
> > from one schema/code-generation separated from others so code and
> > be generated from multiple schemas with clobbering previously
> 
> s/with/without/
> 
> > created code.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  .gitignore                      |   1 +
> >  Makefile                        |   5 +-
> >  docs/qmp-full-introspection.txt |  17 ++++
> >  scripts/qapi-introspect.py      | 172 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 194 insertions(+), 1 deletion(-)
> >  create mode 100644 scripts/qapi-introspect.py
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 1c9d63d..de3cb80 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -22,6 +22,7 @@ linux-headers/asm
> >  qapi-generated
> >  qapi-types.[ch]
> >  qapi-visit.[ch]
> > +qapi-introspect.h
> >  qmp-commands.h
> >  qmp-marshal.c
> >  qemu-doc.html
> > diff --git a/Makefile b/Makefile
> > index bdff4e4..1dac5e7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -45,7 +45,7 @@ endif
> >  endif
> >  
> >  GENERATED_HEADERS = config-host.h qemu-options.def
> > -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> > +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-introspect.h
> >  GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c
> >  
> >  GENERATED_HEADERS += trace/generated-events.h
> > @@ -229,6 +229,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> >  qmp-commands.h qmp-marshal.c :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> >  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> > +qapi-introspect.h:\
> > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-introspect.py $(qapi-py)
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py $(gen-out-type) -o "." < $<, "  GEN   $@")
> >  
> >  QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
> >  $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
> > diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> > index d2cf7b3..8ecbc0c 100644
> > --- a/docs/qmp-full-introspection.txt
> > +++ b/docs/qmp-full-introspection.txt
> > @@ -42,3 +42,20 @@ types.
> >  
> >  'anonymous-struct' will be used to describe arbitrary structs
> >  (dictionary, list or string).
> > +
> > +== Avoid dead loop in recursive extending ==
> > +
> > +We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
> > +that uses themself in their own define data directly or indirectly,
> 
> s/themself/themselves/
> s/define data/definition/
> 
> > +we will not repeatedly extend them to avoid dead loop.
> > +
> > +We use a 'parents List' to record the visit path, type name of each
> > +extended node will be saved to the List.
> > +
> > +Append type name to the list before extending, and remove type name
> > +from the list after extending.
> > +
> > +If the type name is already extended in parents List, we won't extend
> > +it repeatedly for avoiding dead loop.
> 
> This "parents" list detail is not reflected in the generated information,
> right?

Not, it just used to help the extending.

>  I think it's good enough to describe that "type will not be extented
> more than once in a schema, when there's direct or indirect recursive
> type composition".

It's more meaningful.
 
> > +
> > +'recursive' indicates if the type is extended or not.
> > diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> > new file mode 100644
> > index 0000000..03179fa
> > --- /dev/null
> > +++ b/scripts/qapi-introspect.py
> > @@ -0,0 +1,172 @@
> > +#
> > +# QAPI introspection info generator
> > +#
> > +# Copyright (C) 2014 Red Hat, Inc.
> > +#
> > +# Authors:
> > +#  Amos Kong <akong@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPLv2.
> > +# See the COPYING.LIB file in the top-level directory.
> > +
> > +from ordereddict import OrderedDict
> > +from qapi import *
> > +import sys
> > +import os
> > +import getopt
> > +import errno
> > +
> > +
> > +try:
> > +    opts, args = getopt.gnu_getopt(sys.argv[1:], "hp:o:",
> > +                                   ["header", "prefix=", "output-dir="])
> > +except getopt.GetoptError, err:
> > +    print str(err)
> > +    sys.exit(1)
> > +
> > +output_dir = ""
> > +prefix = ""
> > +h_file = 'qapi-introspect.h'
> > +
> > +do_h = False
> > +
> > +for o, a in opts:
> > +    if o in ("-p", "--prefix"):
> > +        prefix = a
> 
> Is this option used in your series?

Not, I will remove it.
 
> Thanks,
> Fam
 
Thanks for your review.
Amos

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

[Fam Zheng]

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 <akong@redhat.com>
> ---
>  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.

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': '...', ...},"
        ...
        ;

 * 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", ...

Fam

> +{
> +    DataObjectMemberList *list = NULL;
> +    QDict *qdict = qobject_to_qdict(data);
> +    QObject *subdata = qdict_get(qdict, "_obj_data");
> +
> +    list = NULL;
> +    if (subdata->type->code == QTYPE_QDICT) {
> +        list = qobject_to_dict_memlist(subdata);
> +    } else if (subdata->type->code == QTYPE_QLIST) {
> +        list = qobject_to_list_memlist(subdata);
> +    }
> +
> +    return list;
> +}
> +
> +static DataObject *qobject_to_dataobj(QObject *data)
> +{
> +    QObject *subdata;
> +    QDict *qdict;
> +    const char *obj_type, *obj_recursive;
> +    DataObject *obj = g_malloc0(sizeof(DataObject));
> +
> +    if (data->type->code == QTYPE_QSTRING) {
> +        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> +        obj->reference_type = g_malloc0(sizeof(String));
> +        obj->reference_type->str = g_strdup(qobject_get_str(data));
> +        return obj;
> +    }
> +
> +    qdict = qobject_to_qdict(data);
> +    assert(qdict != NULL);
> +
> +    obj_type = qobject_get_str(qdict_get(qdict, "_obj_type"));
> +    obj_recursive = qobject_get_str(qdict_get(qdict, "_obj_recursive"));
> +    if (!strcmp(obj_recursive, "True")) {
> +        obj->has_recursive = true;
> +        obj->recursive = true;
> +    }
> +
> +    obj->has_name = true;
> +    obj->name = g_strdup(qobject_get_str(qdict_get(qdict, "_obj_name")));
> +
> +    subdata = qdict_get(qdict, "_obj_data");
> +    qdict = qobject_to_qdict(subdata);
> +
> +    if (!strcmp(obj_type, "command")) {
> +        obj->kind = DATA_OBJECT_KIND_COMMAND;
> +        obj->command = g_malloc0(sizeof(DataObjectCommand));
> +        subdata = qdict_get(qobject_to_qdict(subdata), "data");
> +
> +        if (subdata && subdata->type->code == QTYPE_QDICT) {
> +            obj->command->has_data = true;
> +            obj->command->data = qobject_to_memlist(subdata);
> +        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> +            abort();
> +        }
> +
> +        subdata = qdict_get(qdict, "returns");
> +        if (subdata) {
> +            obj->command->has_returns = true;
> +            obj->command->returns = qobject_to_dataobj(subdata);
> +        }
> +
> +        subdata = qdict_get(qdict, "gen");
> +        if (subdata && subdata->type->code == QTYPE_QSTRING) {
> +            obj->command->has_gen = true;
> +            if (!strcmp(qobject_get_str(subdata), "no")) {
> +                obj->command->gen = false;
> +            } else {
> +                obj->command->gen = true;
> +            }
> +        }
> +    } else if (!strcmp(obj_type, "union")) {
> +        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> +        obj->unionobj = g_malloc0(sizeof(DataObjectUnion));
> +        subdata = qdict_get(qdict, "data");
> +        obj->unionobj->data = qobject_to_memlist(subdata);
> +
> +        subdata = qdict_get(qdict, "base");
> +        if (subdata) {
> +            obj->unionobj->has_base = true;
> +            obj->unionobj->base = qobject_to_dataobj(subdata);
> +        }
> +
> +        subdata = qdict_get(qdict, "discriminator");
> +        if (subdata) {
> +            obj->unionobj->has_discriminator = true;
> +            obj->unionobj->discriminator = qobject_to_dataobj(subdata);
> +        }
> +    } else if (!strcmp(obj_type, "type")) {
> +        obj->kind = DATA_OBJECT_KIND_TYPE;
> +        obj->type = g_malloc0(sizeof(DataObjectType));
> +        subdata = qdict_get(qdict, "data");
> +        if (subdata) {
> +            obj->type->data = qobject_to_memlist(subdata);
> +        }
> +     } else if (!strcmp(obj_type, "enum")) {
> +        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> +        obj->enumeration = g_malloc0(sizeof(DataObjectEnumeration));
> +        subdata = qdict_get(qdict, "data");
> +        obj->enumeration->data = qobject_to_strlist(subdata);
> +    } else {
> +        obj->has_name = false;
> +        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> +        obj->anonymous_struct = g_malloc0(sizeof(DataObjectAnonymousStruct));
> +        obj->anonymous_struct->data = qobject_to_memlist(data);
> +    }
> +
> +    return obj;
> +}
> +
> +DataObjectList *qmp_query_qmp_schema(Error **errp)
> +{
> +    DataObjectList *list = NULL;
> +    DataObjectList **plist = &list;
> +    QObject *data;
> +    int i;
> +
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        assert(data != NULL);
> +        DataObjectList *entry = g_malloc0(sizeof(DataObjectList));
> +        entry->value = qobject_to_dataobj(data);
> +        *plist = entry;
> +        plist = &entry->next;
> +    }
> +
> +    return list;
> +}
> +
>  void qmp_add_client(const char *protocol, const char *fdname,
>                      bool has_skipauth, bool skipauth, bool has_tls, bool tls,
>                      Error **errp)
> -- 
> 1.8.4.2
> 
> 

Re: [Qemu-devel] [PATCH v4 5/5] update docs/qmp-full-introspection.txt

[Paolo Bonzini]

Il 23/01/2014 15:46, Amos Kong ha scritto:
> +The whole schema information will be returned in one go, it contains
> +all the schema entries. It doesn't support to be filtered by type
> +or name. Currently it takes about 4 seconds to return about 1.7M string.
> +Management only needs to execute this command once after installing
> +QEMU package.
> +

This is quite a lot.

Without comments, qapi-schema.json is 27k.  I'd expect the DataObject 
representation to be in the ballpark of 100-200k.

I don't understand why is it necessary to expand all the types in the 
result?

Paolo

Re: [Qemu-devel] [PATCH v4 5/5] update docs/qmp-full-introspection.txt

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

On 01/24/2014 04:43 AM, Paolo Bonzini wrote:
> Il 23/01/2014 15:46, Amos Kong ha scritto:
>> +The whole schema information will be returned in one go, it contains
>> +all the schema entries. It doesn't support to be filtered by type
>> +or name. Currently it takes about 4 seconds to return about 1.7M string.
>> +Management only needs to execute this command once after installing
>> +QEMU package.
>> +

But management has to call the command for every variant of the qemu
binary that it plans on driving - on a system with multiple qemu-* for
targetting multiple target types, this starts to border on unacceptably
long for libvirt.  And while libvirt might use this command to learn
about what is supported for a handful of specific commands, making
libvirt store 1.7M + additional memory for its JSON parse of that data
per qemu starts to add up fast, if libvirt were to keep that data around
for the life of libvirtd rather than just learning what it needs from
the string and discarding the string up front.  We've just made an
argument for WHY we need filtering to just a given type/command.

> 
> This is quite a lot.
> 
> Without comments, qapi-schema.json is 27k.  I'd expect the DataObject
> representation to be in the ballpark of 100-200k.
> 
> I don't understand why is it necessary to expand all the types in the
> result?

Any time a type is returned at the top level in the same query (such as
in your global query), management can look up any unexpanded type in the
same result.  I could understand expanding the results when returning
only a subset of the tree (that is, when filtering is added, asking for
a single type should give me all information about that type, including
the subtypes it references, without me having to make multiple calls to
learn about the subtypes), but even then, it might be worth having an
optional boolean flag on the query that says whether I want expansion
vs. compact results.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Amos Kong]

On Fri, Jan 24, 2014 at 05:34:35PM +0800, Amos Kong wrote:
> On Fri, Jan 24, 2014 at 05:12:12PM +0800, Fam Zheng wrote:
> > On Thu, 01/23 22:46, Amos Kong wrote:

> > > index 0000000..03179fa
> > > --- /dev/null
> > > +++ b/scripts/qapi-introspect.py
> > > @@ -0,0 +1,172 @@
> > > +#
> > > +# QAPI introspection info generator
> > > +#
> > > +# Copyright (C) 2014 Red Hat, Inc.
> > > +#
> > > +# Authors:
> > > +#  Amos Kong <akong@redhat.com>
> > > +#
> > > +# This work is licensed under the terms of the GNU GPLv2.
> > > +# See the COPYING.LIB file in the top-level directory.
> > > +
> > > +from ordereddict import OrderedDict
> > > +from qapi import *
> > > +import sys
> > > +import os
> > > +import getopt
> > > +import errno
> > > +
> > > +
> > > +try:
> > > +    opts, args = getopt.gnu_getopt(sys.argv[1:], "hp:o:",
> > > +                                   ["header", "prefix=", "output-dir="])
> > > +except getopt.GetoptError, err:
> > > +    print str(err)
> > > +    sys.exit(1)
> > > +
> > > +output_dir = ""
> > > +prefix = ""
> > > +h_file = 'qapi-introspect.h'
> > > +
> > > +do_h = False
> > > +
> > > +for o, a in opts:
> > > +    if o in ("-p", "--prefix"):
> > > +        prefix = a
> > 
> > Is this option used in your series?
> 
> Not, I will remove it.

It's not used currently, but it will be used when we add schema query
command for qemu-guest-agent in next step, I will add the -p option
at that time.

-- 
			Amos.

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

[Amos Kong]

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 <akong@redhat.com>
> > ---
> >  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

-- 
			Amos.

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

[Amos Kong]

On Mon, Jan 27, 2014 at 04:17:56PM +0800, Amos Kong wrote:
> 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 <akong@redhat.com>
> > > ---
> > >  qapi-schema.json |  11 +++
> > >  qmp-commands.hx  |  42 +++++++++++
> > >  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 268 insertions(+)


> > > +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'}", 
>         "......",
>         "......",
>         "......"
>     ]
> }

Provide two txt files:
 https://i-kvm.rhcloud.com/static/pub/v5/qapi-introspect.h
 https://i-kvm.rhcloud.com/static/pub/v5/query-output.txt

-- 
			Amos.

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

[Paolo Bonzini]

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 <akong@redhat.com>
>>> ---
>>>  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.

Paolo

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 <akong@redhat.com>
> >>>---
> >>> 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.

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

[Paolo Bonzini]

Il 27/01/2014 11:07, Amos Kong ha scritto:
>> > 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.
>

I think Eric already answered.

In any case, 4 seconds to produce 1.7 MB is a sign of a bug in my 
opinion.  What does the profile say?

Paolo

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

[Fam Zheng]

On Mon, 01/27 10:38, 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 <akong@redhat.com>
> >>>---
> >>> 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.
> 

I think the interface is fine with v4, no need to change to string array. It's
just the implementation in this series shows some duplication:

                  generator in python                         parser in C
qapi-schema.json ----------------------> qapi-introspect.h ------------------> qmp result

When you look at "qmp result", it is qapi-schema.json rewritten in a formal
syntax (a real schema?). But when you look at qapi-introspect.h, that is
generated by python and parsed by C, it's a schema of the qapi-schema. So the
python code and the C code, on the arrows, are just (to a big degree) reversal
to each other, as they both implement the "schema's schema" logic: one for
generation and the other for parsing. They both write code for each type like
"command", "union", "discriminator", etc.

My question is why is this generate-and-parse necessary? Will it be reused in
the future? Can we achieve it with less duplication?

Fam

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

[Amos Kong]

On Mon, Jan 27, 2014 at 06:46:31PM +0800, Fam Zheng wrote:
> On Mon, 01/27 10:38, 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 <akong@redhat.com>
> > >>>---
> > >>> 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.
> > 
> 
> I think the interface is fine with v4, no need to change to string array. It's
> just the implementation in this series shows some duplication:
 
The V4 output is very close to original conclusion about DataObject/metadata.

>                   generator in python                         parser in C
> qapi-schema.json ----------------------> qapi-introspect.h ------------------> qmp result
> 
> When you look at "qmp result", it is qapi-schema.json rewritten in a formal
> syntax (a real schema?). But when you look at qapi-introspect.h, that is
> generated by python and parsed by C, it's a schema of the qapi-schema. So the
> python code and the C code, on the arrows, are just (to a big degree) reversal
> to each other, as they both implement the "schema's schema" logic: one for
> generation and the other for parsing. They both write code for each type like
> "command", "union", "discriminator", etc.

Right, we can't avoid the duplicate if we want to connect multiple
interfaces (Python, QMP, QAPI, JSON).
 
> My question is why is this generate-and-parse necessary?

It's request of Libvirt, actually we can directly return the raw
schema to Libvirt without extending/parsing, then Libvirt parse
by itself.

> Will it be reused in the future?

It seems not.

> Can we achieve it with less duplication?
> 
> Fam

Let's see the feedback of Eric.

Thanks, Amos

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

[Paolo Bonzini]

Il 28/01/2014 11:45, Amos Kong ha scritto:
> > My question is why is this generate-and-parse necessary?
>
> It's request of Libvirt, actually we can directly return the raw
> schema to Libvirt without extending/parsing, then Libvirt parse
> by itself.
>
> > Can we achieve it with less duplication?
>
> Let's see the feedback of Eric.

Eric's feedback is certainly useful, but I think we need to look at it 
from the QEMU perspective more than the libvirt perspective.

Passing the raw schema and letting libvirt parse it is a Really Bad idea 
from the QEMU perspective, in my opinion, even if it means a little more 
work now and even if libvirt is willing to add the parser.

First and foremost, the current "pseudo-JSON" encoding of the schema is 
nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax 
definitely shouldn't percolate to the QAPI documentation.  Using normal 
QAPI structs means that the normal tool for documentation 
(qapi-schema.json doc comments) applies just as well to QAPI schema 
introspection

Second, if one day we were to change the schema representation from 
"pseudo-JSON" to something else, we would have to carry a "pseudo-JSON" 
serializer for backwards compatibility.  Building QAPI structs and 
relying on the normal formatting machinery is very different from 
putting together strings manually.

The schema must be emitted as JSON data, not as a string.  I'm not 
willing to compromise on this point. :)

Paolo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]

On 01/28/2014 04:14 AM, Paolo Bonzini wrote:

>> Let's see the feedback of Eric.
> 
> Eric's feedback is certainly useful, but I think we need to look at it
> from the QEMU perspective more than the libvirt perspective.
> 
> Passing the raw schema and letting libvirt parse it is a Really Bad idea
> from the QEMU perspective, in my opinion, even if it means a little more
> work now and even if libvirt is willing to add the parser.

Libvirt wants to parse formal qapi, not pseudo-JSON.  I still have on my
to-do list to read the v4 schema and make sure that libvirt can live
with it.

> 
> First and foremost, the current "pseudo-JSON" encoding of the schema is
> nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax
> definitely shouldn't percolate to the QAPI documentation.  Using normal
> QAPI structs means that the normal tool for documentation
> (qapi-schema.json doc comments) applies just as well to QAPI schema
> introspection

Agreed - I definitely want the output of the query command to be fully
described by qapi.  Which means we DO have to convert from the
pseudo-JSON of the qapi file into the final formal qapi format.  But the
conversion is known at code generation time, so you should do it as part
of your python code generator, and not repeat the conversion at runtime
in the C code.  That is, the C code should have everything already split
out into the data structures it needs to just output the qapi structure
as documented for the formal qapi definition.

> 
> Second, if one day we were to change the schema representation from
> "pseudo-JSON" to something else, we would have to carry a "pseudo-JSON"
> serializer for backwards compatibility.  Building QAPI structs and
> relying on the normal formatting machinery is very different from
> putting together strings manually.
> 
> The schema must be emitted as JSON data, not as a string.  I'm not
> willing to compromise on this point. :)

Nor am I.  The output MUST be formal JSON, described by the qapi
(whether we change the syntax of qapi-schema.json and tweak the .py code
generators in the future should not matter, the output of the QMP
command will be the same formal JSON).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Fam Zheng]

On Tue, 01/28 06:58, Eric Blake wrote:
> On 01/28/2014 04:14 AM, Paolo Bonzini wrote:
> 
> >> Let's see the feedback of Eric.
> > 
> > Eric's feedback is certainly useful, but I think we need to look at it
> > from the QEMU perspective more than the libvirt perspective.
> > 
> > Passing the raw schema and letting libvirt parse it is a Really Bad idea
> > from the QEMU perspective, in my opinion, even if it means a little more
> > work now and even if libvirt is willing to add the parser.
> 
> Libvirt wants to parse formal qapi, not pseudo-JSON.  I still have on my
> to-do list to read the v4 schema and make sure that libvirt can live
> with it.
> 
> > 
> > First and foremost, the current "pseudo-JSON" encoding of the schema is
> > nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax
> > definitely shouldn't percolate to the QAPI documentation.  Using normal
> > QAPI structs means that the normal tool for documentation
> > (qapi-schema.json doc comments) applies just as well to QAPI schema
> > introspection
> 
> Agreed - I definitely want the output of the query command to be fully
> described by qapi.  Which means we DO have to convert from the
> pseudo-JSON of the qapi file into the final formal qapi format.  But the
> conversion is known at code generation time, so you should do it as part
> of your python code generator, and not repeat the conversion at runtime
> in the C code.  That is, the C code should have everything already split
> out into the data structures it needs to just output the qapi structure
> as documented for the formal qapi definition.
> 

Yes, that's exactly what I mean.

If we use python to generate code for qobject_to_dataobj() or even generate
object_to_$type() for each qapi type, the C code needed will be minimal anyway.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v4 1/5] qapi: introduce DataObject to describe dynamic structs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 8751 bytes --]

On 01/23/2014 07:46 AM, Amos Kong wrote:
> This patch introduced a DataObject union in qapi-schema.json,
> we use it to describe dynamic data structs.
> 
> We will use it in following patches to support to QMP full
> introspection. We have many kinds of schema in json file,
> they all can be described by DataObject.
> 
> This patch also added a doc: qmp-full-introspection.txt,
> QMP introspection releated document will be added into it.

s/releated/related/

> It helps to use the new query command and understand the
> abstract method in describing the dynamic struct.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  docs/qmp-full-introspection.txt |  44 +++++++++++++
>  qapi-schema.json                | 141 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 185 insertions(+)
>  create mode 100644 docs/qmp-full-introspection.txt
> 
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,44 @@
> += Full introspection support for QMP =

Existing practice on doc files is lousy, but it might be nice to make a
copyright/license statement explicit.

> +
> +
> +== Purpose ==
> +
> +Add a new monitor command for management to  query QMP schema

accidental two spaces

> +information, it returns a range of schema structs, which contain the

s/information, it/information. It/

> +useful metadata to help management to check supported features, QMP
> +commands detail, etc.
> +
> +== 'DataObject' union ==
> +
> +{ 'union': 'DataObject',
> +  'base': 'DataObjectBase',
> +  'discriminator': 'type',
> +  'data': {
> +    'anonymous-struct': 'DataObjectAnonymousStruct',
> +    'command': 'DataObjectCommand',
> +    'enumeration': 'DataObjectEnumeration',
> +    'reference-type': 'String',
> +    'type': 'DataObjectType',
> +    'unionobj': 'DataObjectUnion' } }
> +
> +Currently we have schema difinitions for type, command, enumeration,

s/difinitions/definitions/

> +union. Some arbitrary structs (dictionary, list or string) and native
> +types are also used in the body of definitions.
> +
> +Here we use "DataObject" union to abstract all above schema. We want
> +to provide more useful metadata, and used some enum/unions to indicate
> +the dynamic type. In the output, some simple data is processed too
> +unwieldy. In another side, some complex data is described clearly.
> +It's also caused by some limitation of QAPI infrastructure.
> +
> +So we define 'DataObject' to be an union, it always has an object name
> +except anonymous struct.
> +
> +'command', 'enumeration', 'type', 'unionobj' are common schema type,
> +'union' is a build-in type, so I used unionobj here.

s/build-in/built-in/

> +
> +'reference-type' will be used to describe native types and unextended
> +types.
> +
> +'anonymous-struct' will be used to describe arbitrary structs
> +(dictionary, list or string).

Giving some examples from the qapi-schema.json file, and how they are
represented in JSON, would be nice.

> diff --git a/qapi-schema.json b/qapi-schema.json
> index f27c48a..c63f0ca 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4270,3 +4270,144 @@
>  # Since: 1.7
>  ##
>  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> +
> +##
> +# @DataObjectBase
> +#
> +# Base attributes of @DataObject
> +#
> +# @name: #optional @DataObject name
> +# @type: @DataObject type
> +# @recursive: #optional key to indicate if it's extended

I'm not quite sure from the documentation file what an "extended" object is.

> +#
> +# Since: 1.8

2.0 (we aren't doing a 1.8 release)

> +##
> +{ 'type': 'DataObjectBase',
> +  'data': { '*name': 'str', 'type': 'str', '*recursive': 'bool' } }
> +
> +##
> +# @DataObjectMemberType
> +#
> +# Type of @DabaObjectMember

s/Daba/Data/

> +#
> +# @reference: reference string
> +# @anonymous: arbitrary struct
> +# @extend: the @DataObjectMember
> +#
> +# Since: 1.8

2.0

> +##
> +{ 'union': 'DataObjectMemberType',
> +  'discriminator': {},
> +  'data': { 'reference': 'str',
> +            'anonymous': 'DataObject',
> +            'extend': 'DataObject' } }

I'm not sure I follow the difference between anonymous and extend here.

> +
> +##
> +# @DataObjectMember
> +#
> +# General member of @DataObject
> +#
> +# @type: type of @DataObjectMember
> +# @name: #optional name
> +# @optional: #optional key to indicate if the @DataObjectMember is optional
> +# @recursive: #optional key to indicate if it's defined recursively
> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectMember',
> +  'data': { 'type': 'DataObjectMemberType', '*name': 'str',
> +            '*optional': 'bool', '*recursive': 'bool' } }
> +
> +##
> +# @DataObjectAnonymousStruct
> +#
> +# Arbitrary struct, it can be dictionary, list or string
> +#
> +# @data: content of arbitrary struct
> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectAnonymousStruct',
> +  'data': { 'data': [ 'DataObjectMember' ] } }
> +
> +##
> +# @DataObjectCommand
> +#
> +# QMP Command schema
> +#
> +# @data: QMP command content

#optional

> +# @returns: returns of executing command

@returns: #optional type of return value after executing command

> +# @gen: a key to suppress code generation

I'm not sure we want to expose @gen through QAPI.  It's better to omit
it for now, and add it later if we find that it matters, than it is to
expose it now, then wish we hadn't when trying to refactor internal code
generation later.  That is, knowing whether QAPI code resulted in
automatic-generated code instead of hand-written code in qemu won't
affect how libvirt is able to use the interface.

> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectCommand',
> +  'data': { '*data': [ 'DataObjectMember' ],
> +            '*returns': 'DataObject',
> +            '*gen': 'bool' } }
> +
> +##
> +# @DataObjectEnumeration
> +#
> +# Enumeration schema
> +#
> +# @data: enumeration content, it's a string list

s/it's/as/

May be worth a comment that although this is shown as a list, there is
no intrinsic meaning to the relative order of names, and that we
intentionally allow future versions of qemu to reorder and/or inject
enum values in the middle rather than only appending at the end.

> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectEnumeration',
> +  'data': { 'data': [ 'str' ] } }
> +
> +##
> +# @DataObjectType
> +#
> +# Type schema
> +#
> +# @data: defined content of type, it's a dictionary or list
> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectType',
> +  'data': { 'data': [ 'DataObjectMember' ] } }
> +
> +##
> +# @DataObjectUnion
> +#
> +# Union schema
> +#
> +# @data: main content of union
> +# @base: base attributes of union

#optional

> +# @discriminator: union discriminator

#optional

> +#
> +# Since: 1.8

2.0

> +##
> +{ 'type': 'DataObjectUnion',
> +  'data': { 'data': [ 'DataObjectMember' ], '*base': 'DataObject',
> +            '*discriminator': 'DataObject' } }
> +
> +##
> +# @DataObject
> +#
> +# Dynamic data struct, it can be command, enumeration, type, union, arbitrary
> +# struct or native type.
> +#
> +# @anonymous-struct: arbitrary struct, it can be dictionary, list or string
> +# @command: QMP command schema
> +# @enumeration: enumeration schema
> +# @reference-type: native type or unextended type
> +# @type: type schema, it will be extended
> +# @unionobj: union schema, 'union' is a conflicted name, so we use
> +#            unionobj instead
> +#
> +# Since: 1.8

2.0

We also need to start thinking about events, since there is a pending
series adding direct QAPI support for events.

> +##
> +{ 'union': 'DataObject',
> +  'base': 'DataObjectBase',
> +  'discriminator': 'type',
> +  'data': {
> +    'anonymous-struct': 'DataObjectAnonymousStruct',
> +    'command': 'DataObjectCommand',
> +    'enumeration': 'DataObjectEnumeration',
> +    'reference-type': 'String',

Do you need the full-blown 'String' wrapper type, or will the built-in
'str' work here?

> +    'type': 'DataObjectType',

I'm wondering if this would be better named as 'struct', since it is the
counterpart to a union type.

> +    'unionobj': 'DataObjectUnion' } }

Overall, I think this is a fairly decent QAPI representation of what
forms QMP data elements; I'll have a better feel for it after I review
your generated output, but looks like we are on the right track for a
self-describing grammar.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 8149 bytes --]

On 01/23/2014 07:46 AM, Amos Kong wrote:
> This is a code generator for qapi introspection. It will parse
> qapi-schema.json, extend schema definitions and generate a schema
> table with metadata, it references to the new structs which we used
> to describe dynamic data structs.  The metadata will help C code to
> allocate right structs and provide useful information to management
> to checking suported feature and QMP commandline detail. The schema

s/suported/supported/

> table will be saved to qapi-introspect.h.
> 
> The $(prefix) is used to as a namespace to keep the generated code
> from one schema/code-generation separated from others so code and
> be generated from multiple schemas with clobbering previously
> created code.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  .gitignore                      |   1 +
>  Makefile                        |   5 +-
>  docs/qmp-full-introspection.txt |  17 ++++
>  scripts/qapi-introspect.py      | 172 ++++++++++++++++++++++++++++++++++++++++

I am NOT a python expert.  But what I _can_ do is apply this patch and
review the generated code for sanity.

>  4 files changed, 194 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/qapi-introspect.py
> 

> +++ b/docs/qmp-full-introspection.txt
> @@ -42,3 +42,20 @@ types.
>  
>  'anonymous-struct' will be used to describe arbitrary structs
>  (dictionary, list or string).
> +
> +== Avoid dead loop in recursive extending ==

I'm still not convinced whether recursive extending is necessary.

Let's step back and look at the big picture: if libvirt asks for the
ENTIRE schema in one go, then it would be nice to have the
representation as compact as possible (minimal time sending data across
the wire, minimal memory consumed).  Libvirt can then create its own
hash table of type references encountered as it consumes the JSON, and
do its own recursive lookups by reading from its hash table on an
as-needed basis.  Recursive expansion avoids the need to look up type
references, but explodes the size of the data sent over the wire.

On the other hand, if we support filtering by one type, then I agree
that getting all details about that type in one command is nicer than
having to issue a command, notice unknown type references, then issue
followup commands to learn about them.  So in this regards, having qemu
do the expansion minimizes back-and-forth traffic.  BUT, should the
expansion be inlined (ie. the return is an array of 1 type, but that
type contains several further layers of JSON giving the full expansion
of every type referenced), or is it smarter to do the expansion via
returning multiple types even though libvirt only asked about one (as an
example, return an array of 10 types, where the first array entry is the
requested type, and the remaining 9 types fill out the type references
mentioned by the first array entry).

> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
> +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.

s/dead loop/infinite loop/

> +
> +We use a 'parents List' to record the visit path, type name of each
> +extended node will be saved to the List.
> +
> +Append type name to the list before extending, and remove type name
> +from the list after extending.
> +
> +If the type name is already extended in parents List, we won't extend
> +it repeatedly for avoiding dead loop.
> +
> +'recursive' indicates if the type is extended or not.

Ah, now we get to the definition of an extended type that I was asking
about in 1/5.

> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> new file mode 100644
> index 0000000..03179fa
> --- /dev/null
> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,172 @@
> +#
> +# QAPI introspection info generator
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# Authors:
> +#  Amos Kong <akong@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPLv2.

Any reason you can't use GPLv2+ (that is, use the "or later" clause)?
(Red Hat already has blanket approval for any contributions from
employees to be relicensed to GPLv2+, but if you copied code from other
files with a tighter license and non-RH contributor, it's harder to use
that blanket approval, so it's easier to ask you now up front.)

> +fdecl.write(mcgen('''
> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Head file to store parsed information of QAPI schema
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * Authors:
> + *  Amos Kong <akong@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.

Or even license your .py under LGPLv2+, since the generated code is
under that license?

> +def extend_schema(expr, parents=[], member=True):
> +    ret = {}
> +    recu = 'False'
> +    name = ""
> +
> +    if type(expr) is OrderedDict:
> +        if not member:
> +            e = expr.popitem(last=False)
> +            typ = e[0]
> +            name = e[1]
> +        else:
> +            typ = "anonymous-struct"
> +
> +        if typ == 'enum':
> +            for key in expr.keys():
> +                ret[key] = expr[key]
> +        else:
> +            ret = {}
> +            for key in expr.keys():
> +                ret[key], parents = extend_schema(expr[key], parents)

This is where I question whether extend by default is the right action.

> +
> +    elif type(expr) is list:
> +        typ = 'anonymous-struct'
> +        ret = []
> +        for i in expr:
> +            tmp, parents = extend_schema(i, parents)
> +            ret.append(tmp)
> +    elif type(expr) is str:
> +        name = expr
> +        if schema_dict.has_key(expr) and expr not in parents:
> +            parents.append(expr)
> +            typ = schema_dict[expr][1]
> +            recu = 'True'
> +            ret, parents = extend_schema(schema_dict[expr][0].copy(),
> +                                         parents, False)
> +            parents.remove(expr)
> +            ret['_obj_recursive'] = 'True'
> +            return ret, parents
> +        else:
> +            return expr, parents
> +
> +    return {'_obj_member': "%s" % member, '_obj_type': typ,
> +            '_obj_name': name, '_obj_recursive': recu,
> +            '_obj_data': ret}, parents
> +

What's with the leading underscore?

> +
> +exprs = parse_schema(sys.stdin)
> +schema_dict = {}
> +
> +for expr in exprs:
> +    if expr.has_key('type') or expr.has_key('enum') or expr.has_key('union'):
> +        e = expr.copy()
> +

We'll eventually need to cover event types.

> +        first = e.popitem(last=False)
> +        schema_dict[first[1]] = [expr.copy(), first[0]]
> +
> +fdecl.write('''const char *const qmp_schema_table[] = {
> +''')
> +
> +def convert(odict):
> +    d = {}
> +    for k, v in odict.items():
> +        if type(v) is OrderedDict:
> +            d[k] = convert(v)
> +        elif type(v) is list:
> +            l = []
> +            for j in v:
> +                if type(j) is OrderedDict:
> +                    l.append(convert(j))
> +                else:
> +                    l.append(j)
> +            d[k] = l
> +        else:
> +            d[k] = v
> +    return d
> +
> +count = 0
> +for expr in exprs:
> +    fdecl.write('''    /* %s */
> +''' % expr)
> +
> +    expr, parents = extend_schema(expr, [], False)
> +    fdecl.write('''    "%s",
> +
> +''' % convert(expr))
> +
> +fdecl.write('''    NULL };
> +
> +#endif
> +''')
> +
> +fdecl.flush()
> +fdecl.close()
> 

Like I said, I think my review will be more helpful by looking at the
generated file; I'll follow up in another email (probably tomorrow,
since it's now late for me) with more comments once I actually finish that.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 3/5] qobject: introduce qobject_get_str()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On 01/23/2014 07:46 AM, Amos Kong wrote:
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qstring.c          | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)

Is there anything you can add to the testsuite to validate that this
works as expected?  I'm not quite sure if check-qdict.c is the best fit,
but it's the sort of test I have in mind.

> +++ b/qobject/qstring.c
> @@ -135,6 +135,25 @@ const char *qstring_get_str(const QString *qstring)
>  }
>  
>  /**
> + * qobject_to_str(): Convert a QObject to QString and return
> + * a pointer to the stored string
> + */
> +const char *qobject_get_str(const QObject *data)
> +{
> +    QString *qstr;
> +
> +    if (!data) {
> +        return NULL;
> +    }
> +    qstr = qobject_to_qstring(data);
> +    if (qstr) {
> +        return qstring_get_str(qstr);
> +    } else {
> +        return NULL;
> +    }

It looks like this is shorthand for getting at the string value of a
QTYPE_QSTRING object.  I'm not sure how you were planning on using it,
or if it saves much code.  This is where you could use your commit
message to explain why this shorthand will be useful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

On 01/23/2014 07:46 AM, 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:

Do we still want this TODO in the commit message?  It would be nice to
get both of these features (introspection, and events described in qapi)
in for 2.0; I'm trying to spend some review time to help get us there.

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

Yes, we want that too :)  Have you started that patch, or are you trying
to get more feedback on this patch to make sure you're on the right track?

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  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

2.0

And here's where an optional bool on whether to expand may be
worthwhile, as well as an optional argument allowing callers to filter data.

> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }

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

s/comand/command/

> +++ b/qmp.c
> +static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> +{
> +
> +    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));

The blank line here looks unusual.

I didn't look at the code very closely; I want to look at the generated
.h file first.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 8960 bytes --]

On 02/03/2014 05:15 PM, Eric Blake wrote:
> On 01/23/2014 07:46 AM, Amos Kong wrote:
>> This is a code generator for qapi introspection. It will parse
>> qapi-schema.json, extend schema definitions and generate a schema
>> table with metadata, it references to the new structs which we used
>> to describe dynamic data structs.  The metadata will help C code to
>> allocate right structs and provide useful information to management
>> to checking suported feature and QMP commandline detail. The schema
> 
> s/suported/supported/
> 
>> table will be saved to qapi-introspect.h.
>>

> 
> I am NOT a python expert.  But what I _can_ do is apply this patch and
> review the generated code for sanity.


>> +fdecl.write(mcgen('''
>> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> +
>> +/*
>> + * Head file to store parsed information of QAPI schema

s/Head/Header/

> Like I said, I think my review will be more helpful by looking at the
> generated file; I'll follow up in another email (probably tomorrow,
> since it's now late for me) with more comments once I actually finish that.
> 

It took me longer than planned to get back to this.  But here goes my
impressions of the generated file:

-rw-rw-r--. 1 eblake eblake 667643 Feb 10 16:16 qapi-introspect.h
-rw-rw-r--. 1 eblake eblake 126261 Feb  7 17:12 qapi-schema.json
-rw-rw-r--. 1 eblake eblake  80170 Feb  7 17:12 qapi-types.h

Wow, that's a LOT of code.  Why does it take 6 times more C code than
what qapi itself represented everything in?  Are we going too far with
inlining type information?  A larger file MIGHT be okay, if that's what
it takes to make C code that is expressive of the information at hand
(after all, the whole point of our qapi is to give us some shorthand, so
that we can quickly define types in less syntax than C) - but I want to
be sure that we really need that much content.  For comparison, the
generated qapi-types.h is smaller than the input; sure, that's in part
due to the comments being stripped out of the input file, but it's
evidence that the C code representation of qapi should be about the same
size as the input file, not approaching an order of magnitude larger.

const char *const qmp_schema_table[] = {
    /* OrderedDict([('enum', 'ErrorClass'), ('data', ['GenericError',
'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive',
'DeviceNotFound', 'KVMMissingCap'])]) */
    "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
'ErrorClass', '_obj_data': {'data': ['GenericError', 'CommandNotFound',
'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound',
'KVMMissingCap']}, '_obj_recursive': 'False'}",

    /* OrderedDict([('command', 'add_client'), ('data',
OrderedDict([('protocol', 'str'), ('fdname', 'str'), ('*skipauth',
'bool'), ('*tls', 'bool')]))]) */
    "{'_obj_member': 'False', '_obj_type': 'command', '_obj_name':
'add_client', '_obj_data': {'data': {'_obj_type': 'anonymous-struct',
'_obj_member': 'True', '_obj_name': '', '_obj_data': {'*skipauth':
'bool', 'protocol': 'str', 'fdname': 'str', '*tls': 'bool'},
'_obj_recursive': 'False'}}, '_obj_recursive': 'False'}",

Long lines!  Just because it's generated doesn't mean it can't have nice
line wraps.  Make your generator output some strategic whitespace, so
that a human perusing the file stands a chance of understanding it (look
at qapi-types.h for comparison).

No sorting?  This looks like you just dumped the output in hash-table
order.  Please sort the array by type and/or name, so that if we add
filtering, the C code can then do O(log n) lookup via bsearch rather
than an O(n) linear crawl (or maybe even multiple tables, one per type,
with each table sorted by name within that type).

The comments before each string entry is redundant.  Cut your file in
half by listing only what the C compiler cares about - after all the
information is supposed to be self-describing enough that if the comment
actually added anything, we failed at introspecting enough useful
information to the user.

A flat-out array of pre-compiled strings?  I guess it makes generating
the output of your command a little faster (just replay the pre-computed
strings, instead of having to stringify a QObject), but it is lousy if
you ever have to process the data differently.  I was totally expecting
an array of structs.  And not just any struct, but an array of the C
struct that gets generated from the QAPI code.  That is, since qapi is
_already_ the mechanism for generating decent C code structs, and we
want introspection to be self-describing, then your 'struct DataObject'
from qapi-types.h (as generated by patch 1/5) should already be
sufficient as the base of your array.  Or maybe we make an array of yet
one more layer of type:

typedef struct QIntrospection QIntrospection;
struct QIntrospection {
    DataObject data;
    const char *string;
};

so that the C code has access to both the qapi struct and the
pre-rendered string, and can thus get at whichever piece of information
is needed (array[i].data.name when filtering by name, array[i].string
when outputting pre-formatted text).

What I find most appalling about the generated file is that each entry
of the array is a JSON string, but that the JSON string does NOT match
the JSON that would be sent over the wire when sending a DataObject.
Thus, the only way your generated file is usable is if you run the
string through a JSON parser, peel out the portions of the string you
need, and then reconstruct things back into the QMP command.  You REALLY
want either a DataObject[] or a QIntrospection[], so that you DON'T have
to parse strings in your C code.  The python code should have already
generated the header file with everything already placed in C structs
and/or QMP wire format strings in the format that best suits your needs!

That is, I think you want something more like this (rendering the first
two lines that I quoted above in pseudo-C):

const DataObject qmp_schema_table[] = {
    { .has_name = true,
      .name = "ErrorClass",
      .kind = DATA_OBJECT_KIND_ENUMERATION,
      .enumeration = { .value = "GenericError",
             .next = { .value = "CommandNotFound",
             .next = { .value = "DeviceEncrypted",
             .next = { .value = "DeviceNotActive",
             .next = { .value = "DeviceNotFound",
             .next = { .value = "KVMMissingCap",
             .next = NULL } } } } } },
      .has_recursive = false,
    },
    { .has_name = true,
      .name = "add_client",
      .kind = DATA_OBJECT_KIND_COMMAND,
      .command = {
          .has_data = true,
          .data = { .value = { .type = { .kind =
DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE, .reference = "str" },
                               .has_name = true,
                               .name = "protocol",
                               .has_optional = false,
                               .has_recursive = false },
          .next = { .value = { .type = { .kind =
DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE, .reference = "bool" },
                               .has_name = true,
                               .name = "skipauth",
                               .has_optional = true,
                               .optional = true,
                               .has_recursive = false },
          ...
          .next = NULL }...},
          .has_returns = false,
          .has_gen = false,
        },
      .has_recursive = false,
    },

and so on.  Of course, as I typed that, I realize that you can't
actually initialize DataObjectCommand* and other object pointers via
simple {} initializers; so you actually need to be more verbose and use
some C99 typed initializers:

...
.enumeration = &(DataObjectEnumeration) { .value = "GenericError",
       .next = &(DataObjectEnumeration) { .value = "CommandNotFound",
...

Or maybe all you need is:

const QIntrospection qmp_schema_table[] = {
    { .name = "ErrorClass",
      .str = "{\"name\":\"ErrorClass\","
              "\"type\":\"enumeration\","
              "\"data\":["
                 "\"GenericError\","
                 "\"CommandNotFound\","
                 "\"DeviceEncrypted\","
                 "\"DeviceNotFound\","
                 "\"KVMMissingCap\""
              "]}"
    },
    { .name = "add_client",
      .str = "{\"name\":\"add_client\","
              "\"type\":\"command\","
              "\"data\":{"
                ...

with just the object name and pre-rendered string in each array entry.
But my point remains - let the python code generate something USEFUL,
and not something that the C code has to re-parse.  If you're going to
store a string, store it in the format that QMP will already hand over
the wire.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]