[Qemu-devel] [PATCH v3 0/3] QMP full introspection

[Qemu-devel] [PATCH v3 0/3] QMP full introspection

[Amos Kong]

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

The whole output of query-qmp-schema command:
 https://raw.github.com/kongove/misc/master/txt/qmp-introspection.output.txt

Welcome your comments!

V2: use 'DataObject' to describe dynamic struct
V3: improve the metadata as suggested by eric

Amos Kong (3):
  qapi: cleanup redundant variable
  qapi: change qapi to convert schema json
  qmp: full introspection support for QMP

 Makefile                        |   5 +-
 docs/qmp-full-introspection.txt |  97 ++++++++++
 qapi-schema.json                | 150 ++++++++++++++++
 qmp-commands.hx                 |  43 ++++-
 qmp.c                           | 382 ++++++++++++++++++++++++++++++++++++++++
 scripts/qapi-commands.py        |   2 +-
 scripts/qapi-types.py           |  48 ++++-
 scripts/qapi-visit.py           |   2 +-
 scripts/qapi.py                 |  23 ++-
 9 files changed, 737 insertions(+), 15 deletions(-)
 create mode 100644 docs/qmp-full-introspection.txt

-- 
1.8.4.2

[Qemu-devel] [PATCH v3 1/3] qapi: cleanup redundant variable

[Amos Kong]

No need to re-append an expr list, it's ok to return schema.exprs

Signed-off-by: Amos Kong <akong@redhat.com>
---
 scripts/qapi.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 750e9fb..4263bbd 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -165,8 +165,6 @@ def parse_schema(fp):
         print >>sys.stderr, e
         exit(1)
 
-    exprs = []
-
     for expr in schema.exprs:
         if expr.has_key('enum'):
             add_enum(expr['enum'])
@@ -175,9 +173,8 @@ def parse_schema(fp):
             add_enum('%sKind' % expr['union'])
         elif expr.has_key('type'):
             add_struct(expr)
-        exprs.append(expr)
 
-    return exprs
+    return schema.exprs
 
 def parse_args(typeinfo):
     if isinstance(typeinfo, basestring):
-- 
1.8.4.2

[Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Amos Kong]

QMP schema is defined in a json file, it will be parsed by
qapi scripts and generate C files.

We want to return the schema information to management,
this patch converts the json file to a string table in a
C head file, then we can use the json content in QEMU code.

eg: (qmp-schema.h)
  const char *const qmp_schema_table[] = {
    "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
    "{ 'command': 'query-name', 'returns': 'NameInfo' }",
    ...
  }

Signed-off-by: Amos Kong <akong@redhat.com>
---
 Makefile                 |  5 ++++-
 scripts/qapi-commands.py |  2 +-
 scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 scripts/qapi-visit.py    |  2 +-
 scripts/qapi.py          | 20 +++++++++++++++-----
 5 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index bdff4e4..2c29755 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 qmp-schema.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   $@")
+qmp-schema.h:\
+$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  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/scripts/qapi-commands.py b/scripts/qapi-commands.py
index b12b696..5f4fb94 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -440,7 +440,7 @@ except os.error, e:
     if e.errno != errno.EEXIST:
         raise
 
-exprs = parse_schema(sys.stdin)
+exprs = parse_schema(sys.stdin)[0]
 commands = filter(lambda expr: expr.has_key('command'), exprs)
 commands = filter(lambda expr: not expr.has_key('gen'), commands)
 
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4a1652b..0f86b95 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -15,6 +15,7 @@ import sys
 import os
 import getopt
 import errno
+import re
 
 def generate_fwd_struct(name, members, builtin_type=False):
     if builtin_type:
@@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
 
 
 try:
-    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
+    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
                                    ["source", "header", "builtins",
-                                    "prefix=", "output-dir="])
+                                    "schema-dump-file=", "prefix=",
+                                    "output-dir="])
 except getopt.GetoptError, err:
     print str(err)
     sys.exit(1)
@@ -293,6 +295,7 @@ output_dir = ""
 prefix = ""
 c_file = 'qapi-types.c'
 h_file = 'qapi-types.h'
+schema_dump_file = ""
 
 do_c = False
 do_h = False
@@ -309,11 +312,17 @@ for o, a in opts:
         do_h = True
     elif o in ("-b", "--builtins"):
         do_builtins = True
+    elif o in ("-s", "--schema-dump-file"):
+        schema_dump_file = a
 
 if not do_c and not do_h:
     do_c = True
     do_h = True
 
+if schema_dump_file:
+    do_c = False
+    do_h = False
+
 c_file = output_dir + prefix + c_file
 h_file = output_dir + prefix + h_file
 
@@ -381,7 +390,40 @@ fdecl.write(mcgen('''
 ''',
                   guard=guardname(h_file)))
 
-exprs = parse_schema(sys.stdin)
+exprs_all = parse_schema(sys.stdin)
+
+schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+
+/*
+ * Schema json string table converted from qapi-schema.json
+ *
+ * Copyright (c) 2013 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.
+ *
+ */
+
+const char *const qmp_schema_table[] = {
+"""
+
+if schema_dump_file:
+    for line in exprs_all[1]:
+        line = re.sub(r'#.*\n', ' ', line.strip())
+        line = re.sub(r'\n', ' ', line.strip())
+        line = re.sub(r' +', ' ', line)
+        schema_table += '  "%s",\n' % (line)
+
+    schema_table += '  NULL };\n'
+    f = open(schema_dump_file, "w")
+    f.write(schema_table)
+    f.flush()
+    f.close()
+
+exprs = exprs_all[0]
 exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
 
 fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 65f1a54..db68084 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -480,7 +480,7 @@ fdecl.write(mcgen('''
 ''',
                   prefix=prefix, guard=guardname(h_file)))
 
-exprs = parse_schema(sys.stdin)
+exprs = parse_schema(sys.stdin)[0]
 
 # to avoid header dependency hell, we always generate declarations
 # for built-in types in our header files and simply guard them
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4263bbd..43cc401 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -61,10 +61,13 @@ class QAPISchema:
             self.src += '\n'
         self.cursor = 0
         self.exprs = []
+        self.raw_exprs = []
         self.accept()
 
         while self.tok != None:
-            self.exprs.append(self.get_expr(False))
+            self.cur_entry= ''
+            self.exprs.append(self.get_expr(False, True))
+            self.raw_exprs.append(self.cur_entry)
 
     def accept(self):
         while True:
@@ -103,9 +106,11 @@ class QAPISchema:
             elif not self.tok.isspace():
                 raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
 
-    def get_members(self):
+    def get_members(self, start=None):
         expr = OrderedDict()
         if self.tok == '}':
+            if start != None:
+                self.cur_entry = self.src[start:self.cursor]
             self.accept()
             return expr
         if self.tok != "'":
@@ -118,6 +123,8 @@ class QAPISchema:
             self.accept()
             expr[key] = self.get_expr(True)
             if self.tok == '}':
+                if start != None:
+                    self.cur_entry = self.src[start:self.cursor]
                 self.accept()
                 return expr
             if self.tok != ',':
@@ -142,12 +149,15 @@ class QAPISchema:
                 raise QAPISchemaError(self, 'Expected "," or "]"')
             self.accept()
 
-    def get_expr(self, nested):
+    def get_expr(self, nested, first=False):
         if self.tok != '{' and not nested:
             raise QAPISchemaError(self, 'Expected "{"')
         if self.tok == '{':
+            start = None
+            if first:
+                start = self.cursor - 1
             self.accept()
-            expr = self.get_members()
+            expr = self.get_members(start)
         elif self.tok == '[':
             self.accept()
             expr = self.get_values()
@@ -174,7 +184,7 @@ def parse_schema(fp):
         elif expr.has_key('type'):
             add_struct(expr)
 
-    return schema.exprs
+    return schema.exprs, schema.raw_exprs
 
 def parse_args(typeinfo):
     if isinstance(typeinfo, basestring):
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Fam Zheng]

On 2014年01月05日 20:02, Amos Kong wrote:
> QMP schema is defined in a json file, it will be parsed by
> qapi scripts and generate C files.
>
> We want to return the schema information to management,
> this patch converts the json file to a string table in a
> C head file, then we can use the json content in QEMU code.
>
> eg: (qmp-schema.h)
>    const char *const qmp_schema_table[] = {
>      "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
>      "{ 'command': 'query-name', 'returns': 'NameInfo' }",
>      ...
>    }
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>   Makefile                 |  5 ++++-
>   scripts/qapi-commands.py |  2 +-
>   scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>   scripts/qapi-visit.py    |  2 +-
>   scripts/qapi.py          | 20 +++++++++++++++-----
>   5 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index bdff4e4..2c29755 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 qmp-schema.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   $@")
> +qmp-schema.h:\
> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  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/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index b12b696..5f4fb94 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -440,7 +440,7 @@ except os.error, e:
>       if e.errno != errno.EEXIST:
>           raise
>
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.stdin)[0]
>   commands = filter(lambda expr: expr.has_key('command'), exprs)
>   commands = filter(lambda expr: not expr.has_key('gen'), commands)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a1652b..0f86b95 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -15,6 +15,7 @@ import sys
>   import os
>   import getopt
>   import errno
> +import re
>
>   def generate_fwd_struct(name, members, builtin_type=False):
>       if builtin_type:
> @@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
>
>
>   try:
> -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> +    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
>                                      ["source", "header", "builtins",
> -                                    "prefix=", "output-dir="])
> +                                    "schema-dump-file=", "prefix=",
> +                                    "output-dir="])
>   except getopt.GetoptError, err:
>       print str(err)
>       sys.exit(1)
> @@ -293,6 +295,7 @@ output_dir = ""
>   prefix = ""
>   c_file = 'qapi-types.c'
>   h_file = 'qapi-types.h'
> +schema_dump_file = ""
>
>   do_c = False
>   do_h = False
> @@ -309,11 +312,17 @@ for o, a in opts:
>           do_h = True
>       elif o in ("-b", "--builtins"):
>           do_builtins = True
> +    elif o in ("-s", "--schema-dump-file"):
> +        schema_dump_file = a
>
>   if not do_c and not do_h:
>       do_c = True
>       do_h = True
>
> +if schema_dump_file:
> +    do_c = False
> +    do_h = False
> +
>   c_file = output_dir + prefix + c_file
>   h_file = output_dir + prefix + h_file
>
> @@ -381,7 +390,40 @@ fdecl.write(mcgen('''
>   ''',
>                     guard=guardname(h_file)))
>
> -exprs = parse_schema(sys.stdin)
> +exprs_all = parse_schema(sys.stdin)
> +
> +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Schema json string table converted from qapi-schema.json
> + *
> + * Copyright (c) 2013 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.
> + *
> + */
> +
> +const char *const qmp_schema_table[] = {
> +"""
> +
> +if schema_dump_file:
> +    for line in exprs_all[1]:
> +        line = re.sub(r'#.*\n', ' ', line.strip())
> +        line = re.sub(r'\n', ' ', line.strip())
> +        line = re.sub(r' +', ' ', line)
> +        schema_table += '  "%s",\n' % (line)
> +
> +    schema_table += '  NULL };\n'
> +    f = open(schema_dump_file, "w")
> +    f.write(schema_table)
> +    f.flush()
> +    f.close()
> +
> +exprs = exprs_all[0]
>   exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>
>   fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 65f1a54..db68084 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -480,7 +480,7 @@ fdecl.write(mcgen('''
>   ''',
>                     prefix=prefix, guard=guardname(h_file)))
>
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.stdin)[0]
>
>   # to avoid header dependency hell, we always generate declarations
>   # for built-in types in our header files and simply guard them
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4263bbd..43cc401 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -61,10 +61,13 @@ class QAPISchema:
>               self.src += '\n'
>           self.cursor = 0
>           self.exprs = []
> +        self.raw_exprs = []
>           self.accept()
>
>           while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            self.cur_entry= ''

Missing whitespace before "=".

> +            self.exprs.append(self.get_expr(False, True))
> +            self.raw_exprs.append(self.cur_entry)
>
>       def accept(self):
>           while True:
> @@ -103,9 +106,11 @@ class QAPISchema:
>               elif not self.tok.isspace():
>                   raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
>
> -    def get_members(self):
> +    def get_members(self, start=None):
>           expr = OrderedDict()
>           if self.tok == '}':
> +            if start != None:
> +                self.cur_entry = self.src[start:self.cursor]
>               self.accept()
>               return expr
>           if self.tok != "'":
> @@ -118,6 +123,8 @@ class QAPISchema:
>               self.accept()
>               expr[key] = self.get_expr(True)
>               if self.tok == '}':
> +                if start != None:
> +                    self.cur_entry = self.src[start:self.cursor]
>                   self.accept()
>                   return expr
>               if self.tok != ',':
> @@ -142,12 +149,15 @@ class QAPISchema:
>                   raise QAPISchemaError(self, 'Expected "," or "]"')
>               self.accept()
>
> -    def get_expr(self, nested):
> +    def get_expr(self, nested, first=False):
>           if self.tok != '{' and not nested:
>               raise QAPISchemaError(self, 'Expected "{"')
>           if self.tok == '{':
> +            start = None
> +            if first:
> +                start = self.cursor - 1
>               self.accept()
> -            expr = self.get_members()
> +            expr = self.get_members(start)
>           elif self.tok == '[':
>               self.accept()
>               expr = self.get_values()
> @@ -174,7 +184,7 @@ def parse_schema(fp):
>           elif expr.has_key('type'):
>               add_struct(expr)
>
> -    return schema.exprs
> +    return schema.exprs, schema.raw_exprs
>
>   def parse_args(typeinfo):
>       if isinstance(typeinfo, basestring):
>

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Fam Zheng]

On 2014年01月05日 20:02, Amos Kong wrote:
> QMP schema is defined in a json file, it will be parsed by
> qapi scripts and generate C files.
>
> We want to return the schema information to management,
> this patch converts the json file to a string table in a
> C head file, then we can use the json content in QEMU code.
>
> eg: (qmp-schema.h)
>    const char *const qmp_schema_table[] = {
>      "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
>      "{ 'command': 'query-name', 'returns': 'NameInfo' }",
>      ...
>    }
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>   Makefile                 |  5 ++++-
>   scripts/qapi-commands.py |  2 +-
>   scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>   scripts/qapi-visit.py    |  2 +-
>   scripts/qapi.py          | 20 +++++++++++++++-----
>   5 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index bdff4e4..2c29755 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 qmp-schema.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   $@")
> +qmp-schema.h:\
> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  GEN   $@")
>

It would be nice to also add this file to .gitignore together with this 
patch.

Fam

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Luiz Capitulino]

On Sun,  5 Jan 2014 20:02:30 +0800
Amos Kong <akong@redhat.com> wrote:

> QMP schema is defined in a json file, it will be parsed by
> qapi scripts and generate C files.
> 
> We want to return the schema information to management,
> this patch converts the json file to a string table in a
> C head file, then we can use the json content in QEMU code.
> 
> eg: (qmp-schema.h)
>   const char *const qmp_schema_table[] = {
>     "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
>     "{ 'command': 'query-name', 'returns': 'NameInfo' }",
>     ...
>   }
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  Makefile                 |  5 ++++-
>  scripts/qapi-commands.py |  2 +-
>  scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>  scripts/qapi-visit.py    |  2 +-
>  scripts/qapi.py          | 20 +++++++++++++++-----
>  5 files changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index bdff4e4..2c29755 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 qmp-schema.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   $@")
> +qmp-schema.h:\
> +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  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/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index b12b696..5f4fb94 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -440,7 +440,7 @@ except os.error, e:
>      if e.errno != errno.EEXIST:
>          raise
>  
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.stdin)[0]
>  commands = filter(lambda expr: expr.has_key('command'), exprs)
>  commands = filter(lambda expr: not expr.has_key('gen'), commands)
>  
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4a1652b..0f86b95 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -15,6 +15,7 @@ import sys
>  import os
>  import getopt
>  import errno
> +import re
>  
>  def generate_fwd_struct(name, members, builtin_type=False):
>      if builtin_type:
> @@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
>  
>  
>  try:
> -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> +    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
>                                     ["source", "header", "builtins",
> -                                    "prefix=", "output-dir="])
> +                                    "schema-dump-file=", "prefix=",
> +                                    "output-dir="])
>  except getopt.GetoptError, err:
>      print str(err)
>      sys.exit(1)
> @@ -293,6 +295,7 @@ output_dir = ""
>  prefix = ""
>  c_file = 'qapi-types.c'
>  h_file = 'qapi-types.h'
> +schema_dump_file = ""
>  
>  do_c = False
>  do_h = False
> @@ -309,11 +312,17 @@ for o, a in opts:
>          do_h = True
>      elif o in ("-b", "--builtins"):
>          do_builtins = True
> +    elif o in ("-s", "--schema-dump-file"):
> +        schema_dump_file = a

Instead of adding this to qapi-types.py, wouldn't it be clearer to add
a qapi-dump.py file instead?

Also, I think it would be interesting to split this patch into two. The first
patch changes qapi.py (and related files), this will allow you to better
describe your changes to that file. The second patch adds qapi-dump.py.

In general, this looks good to me but I haven't looked into the
changes done in qapi.py in detail.

>  
>  if not do_c and not do_h:
>      do_c = True
>      do_h = True
>  
> +if schema_dump_file:
> +    do_c = False
> +    do_h = False
> +
>  c_file = output_dir + prefix + c_file
>  h_file = output_dir + prefix + h_file
>  
> @@ -381,7 +390,40 @@ fdecl.write(mcgen('''
>  ''',
>                    guard=guardname(h_file)))
>  
> -exprs = parse_schema(sys.stdin)
> +exprs_all = parse_schema(sys.stdin)
> +
> +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Schema json string table converted from qapi-schema.json
> + *
> + * Copyright (c) 2013 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.
> + *
> + */
> +
> +const char *const qmp_schema_table[] = {
> +"""
> +
> +if schema_dump_file:
> +    for line in exprs_all[1]:
> +        line = re.sub(r'#.*\n', ' ', line.strip())
> +        line = re.sub(r'\n', ' ', line.strip())
> +        line = re.sub(r' +', ' ', line)
> +        schema_table += '  "%s",\n' % (line)
> +
> +    schema_table += '  NULL };\n'
> +    f = open(schema_dump_file, "w")
> +    f.write(schema_table)
> +    f.flush()
> +    f.close()
> +
> +exprs = exprs_all[0]
>  exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>  
>  fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 65f1a54..db68084 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -480,7 +480,7 @@ fdecl.write(mcgen('''
>  ''',
>                    prefix=prefix, guard=guardname(h_file)))
>  
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.stdin)[0]
>  
>  # to avoid header dependency hell, we always generate declarations
>  # for built-in types in our header files and simply guard them
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4263bbd..43cc401 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -61,10 +61,13 @@ class QAPISchema:
>              self.src += '\n'
>          self.cursor = 0
>          self.exprs = []
> +        self.raw_exprs = []
>          self.accept()
>  
>          while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            self.cur_entry= ''
> +            self.exprs.append(self.get_expr(False, True))
> +            self.raw_exprs.append(self.cur_entry)
>  
>      def accept(self):
>          while True:
> @@ -103,9 +106,11 @@ class QAPISchema:
>              elif not self.tok.isspace():
>                  raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
>  
> -    def get_members(self):
> +    def get_members(self, start=None):
>          expr = OrderedDict()
>          if self.tok == '}':
> +            if start != None:
> +                self.cur_entry = self.src[start:self.cursor]
>              self.accept()
>              return expr
>          if self.tok != "'":
> @@ -118,6 +123,8 @@ class QAPISchema:
>              self.accept()
>              expr[key] = self.get_expr(True)
>              if self.tok == '}':
> +                if start != None:
> +                    self.cur_entry = self.src[start:self.cursor]
>                  self.accept()
>                  return expr
>              if self.tok != ',':
> @@ -142,12 +149,15 @@ class QAPISchema:
>                  raise QAPISchemaError(self, 'Expected "," or "]"')
>              self.accept()
>  
> -    def get_expr(self, nested):
> +    def get_expr(self, nested, first=False):
>          if self.tok != '{' and not nested:
>              raise QAPISchemaError(self, 'Expected "{"')
>          if self.tok == '{':
> +            start = None
> +            if first:
> +                start = self.cursor - 1
>              self.accept()
> -            expr = self.get_members()
> +            expr = self.get_members(start)
>          elif self.tok == '[':
>              self.accept()
>              expr = self.get_values()
> @@ -174,7 +184,7 @@ def parse_schema(fp):
>          elif expr.has_key('type'):
>              add_struct(expr)
>  
> -    return schema.exprs
> +    return schema.exprs, schema.raw_exprs
>  
>  def parse_args(typeinfo):
>      if isinstance(typeinfo, basestring):

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Amos Kong]

On Wed, Jan 22, 2014 at 01:06:51PM -0500, Luiz Capitulino wrote:
> On Sun,  5 Jan 2014 20:02:30 +0800
> Amos Kong <akong@redhat.com> wrote:
> 
> > QMP schema is defined in a json file, it will be parsed by
> > qapi scripts and generate C files.
> > 
> > We want to return the schema information to management,
> > this patch converts the json file to a string table in a
> > C head file, then we can use the json content in QEMU code.
> > 
> > eg: (qmp-schema.h)
> >   const char *const qmp_schema_table[] = {
> >     "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
> >     "{ 'command': 'query-name', 'returns': 'NameInfo' }",
> >     ...
> >   }
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  Makefile                 |  5 ++++-
> >  scripts/qapi-commands.py |  2 +-
> >  scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> >  scripts/qapi-visit.py    |  2 +-
> >  scripts/qapi.py          | 20 +++++++++++++++-----
> >  5 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index bdff4e4..2c29755 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 qmp-schema.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   $@")
> > +qmp-schema.h:\
> > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  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/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index b12b696..5f4fb94 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -440,7 +440,7 @@ except os.error, e:
> >      if e.errno != errno.EEXIST:
> >          raise
> >  
> > -exprs = parse_schema(sys.stdin)
> > +exprs = parse_schema(sys.stdin)[0]
> >  commands = filter(lambda expr: expr.has_key('command'), exprs)
> >  commands = filter(lambda expr: not expr.has_key('gen'), commands)
> >  
> > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > index 4a1652b..0f86b95 100644
> > --- a/scripts/qapi-types.py
> > +++ b/scripts/qapi-types.py
> > @@ -15,6 +15,7 @@ import sys
> >  import os
> >  import getopt
> >  import errno
> > +import re
> >  
> >  def generate_fwd_struct(name, members, builtin_type=False):
> >      if builtin_type:
> > @@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
> >  
> >  
> >  try:
> > -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> > +    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
> >                                     ["source", "header", "builtins",
> > -                                    "prefix=", "output-dir="])
> > +                                    "schema-dump-file=", "prefix=",
> > +                                    "output-dir="])
> >  except getopt.GetoptError, err:
> >      print str(err)
> >      sys.exit(1)
> > @@ -293,6 +295,7 @@ output_dir = ""
> >  prefix = ""
> >  c_file = 'qapi-types.c'
> >  h_file = 'qapi-types.h'
> > +schema_dump_file = ""
> >  
> >  do_c = False
> >  do_h = False
> > @@ -309,11 +312,17 @@ for o, a in opts:
> >          do_h = True
> >      elif o in ("-b", "--builtins"):
> >          do_builtins = True
> > +    elif o in ("-s", "--schema-dump-file"):
> > +        schema_dump_file = a
> 
> Instead of adding this to qapi-types.py, wouldn't it be clearer to add
> a qapi-dump.py file instead?

I used scripts/qapi-introspect.py to generate qapi-introspect.h.
Q: qapi-introspect.py or qapi-dump.py? which one is better?

It also helps to extend schema and generate a nested dictionaries with
metadata, it's very simple to use python to extend.

/* OrderedDict([('command', 'query-name'), ('returns', 'NameInfo')]) */
    "{'_obj_member': 'False', '_obj_type': 'command', '_obj_name': 'query-name', '_obj_data': {'returns': {'_obj_type': 'type', '_obj_member': 'False', '_obj_name': 'NameInfo', '_obj_data': {'data': {'_obj_type': 'anonymous-struct', '_obj_member': 'True', '_obj_name': '', '_obj_data': {'*name': 'str'}, '_obj_recursion': 'false'}}, '_obj_recursion': 'true'}}, '_obj_recursion': 'false'}",

Then in qmp.c, we only need to visit the dictionaries and fill the data
to allocated structs, which will be returned to QMP monitor.
 
> Also, I think it would be interesting to split this patch into two. The first
> patch changes qapi.py (and related files), this will allow you to better
> describe your changes to that file. The second patch adds qapi-dump.py.
> 
> In general, this looks good to me but I haven't looked into the
> changes done in qapi.py in detail.
 
In v3, we just change qapi.py:parse_schema() to additionally return raw json string.
In my latest patches, we don't need to change qapi.py, we can directly use OrderedDicts.

I'm working in qmp.c part, maybe we can try to simple DataObject definitions in V4.

BTW, no need to review v3, please wait my refreshed V4 :-)

Thanks, Amos

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Amos Kong]

On Thu, Jan 23, 2014 at 11:05:06AM +0800, Amos Kong wrote:
> On Wed, Jan 22, 2014 at 01:06:51PM -0500, Luiz Capitulino wrote:
> > On Sun,  5 Jan 2014 20:02:30 +0800
> > Amos Kong <akong@redhat.com> wrote:

> > >  do_c = False
> > >  do_h = False
> > > @@ -309,11 +312,17 @@ for o, a in opts:
> > >          do_h = True
> > >      elif o in ("-b", "--builtins"):
> > >          do_builtins = True
> > > +    elif o in ("-s", "--schema-dump-file"):
> > > +        schema_dump_file = a
> > 
> > Instead of adding this to qapi-types.py, wouldn't it be clearer to add
> > a qapi-dump.py file instead?
> 
> I used scripts/qapi-introspect.py to generate qapi-introspect.h.
> Q: qapi-introspect.py or qapi-dump.py? which one is better?
> 
> It also helps to extend schema and generate a nested dictionaries with
> metadata, it's very simple to use python to extend.
> 
> /* OrderedDict([('command', 'query-name'), ('returns', 'NameInfo')]) */
>     "{'_obj_member': 'False', '_obj_type': 'command', '_obj_name': 'query-name', '_obj_data': {'returns': {'_obj_type': 'type', '_obj_member': 'False', '_obj_name': 'NameInfo', '_obj_data': {'data': {'_obj_type': 'anonymous-struct', '_obj_member': 'True', '_obj_name': '', '_obj_data': {'*name': 'str'}, '_obj_recursion': 'false'}}, '_obj_recursion': 'true'}}, '_obj_recursion': 'false'}",
> 
> Then in qmp.c, we only need to visit the dictionaries and fill the data
> to allocated structs, which will be returned to QMP monitor.


The return data to QMP monitor is dynamically allocated, we can't
generate some static struct/table reference to the OrderedDicts.

I tried to generate "qmp_query_qmp_schema(Error **errp)" in qap-introspect.h. 
Python generates some redundant g_malloc0() code when visit all the OrderedDicts
It's a little bit ugly, no performance effects. But it's failed, we have
to use recursion functions to visit nested dictionaries.

So I continue to add qmp_query_qmp_schema() in qmp.c. For reduceing the
C work in running time, I also tried to parse out the metadata in Python.

  
> > Also, I think it would be interesting to split this patch into two. The first
> > patch changes qapi.py (and related files), this will allow you to better
> > describe your changes to that file. The second patch adds qapi-dump.py.
> > 
> > In general, this looks good to me but I haven't looked into the
> > changes done in qapi.py in detail.
>  
> In v3, we just change qapi.py:parse_schema() to additionally return raw json string.
> In my latest patches, we don't need to change qapi.py, we can directly use OrderedDicts.
> 
> I'm working in qmp.c part, maybe we can try to simple DataObject definitions in V4.
> 
> BTW, no need to review v3, please wait my refreshed V4 :-)
> 
> Thanks, Amos
 

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 2/3] qapi: change qapi to convert schema json

[Luiz Capitulino]

On Thu, 23 Jan 2014 11:05:06 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Jan 22, 2014 at 01:06:51PM -0500, Luiz Capitulino wrote:
> > On Sun,  5 Jan 2014 20:02:30 +0800
> > Amos Kong <akong@redhat.com> wrote:
> > 
> > > QMP schema is defined in a json file, it will be parsed by
> > > qapi scripts and generate C files.
> > > 
> > > We want to return the schema information to management,
> > > this patch converts the json file to a string table in a
> > > C head file, then we can use the json content in QEMU code.
> > > 
> > > eg: (qmp-schema.h)
> > >   const char *const qmp_schema_table[] = {
> > >     "{ 'type': 'NameInfo', 'data': {'*name': 'str'} }",
> > >     "{ 'command': 'query-name', 'returns': 'NameInfo' }",
> > >     ...
> > >   }
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  Makefile                 |  5 ++++-
> > >  scripts/qapi-commands.py |  2 +-
> > >  scripts/qapi-types.py    | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> > >  scripts/qapi-visit.py    |  2 +-
> > >  scripts/qapi.py          | 20 +++++++++++++++-----
> > >  5 files changed, 66 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index bdff4e4..2c29755 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 qmp-schema.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   $@")
> > > +qmp-schema.h:\
> > > +$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> > > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py $(gen-out-type) -o "." -s "$@" < $<, "  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/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > > index b12b696..5f4fb94 100644
> > > --- a/scripts/qapi-commands.py
> > > +++ b/scripts/qapi-commands.py
> > > @@ -440,7 +440,7 @@ except os.error, e:
> > >      if e.errno != errno.EEXIST:
> > >          raise
> > >  
> > > -exprs = parse_schema(sys.stdin)
> > > +exprs = parse_schema(sys.stdin)[0]
> > >  commands = filter(lambda expr: expr.has_key('command'), exprs)
> > >  commands = filter(lambda expr: not expr.has_key('gen'), commands)
> > >  
> > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> > > index 4a1652b..0f86b95 100644
> > > --- a/scripts/qapi-types.py
> > > +++ b/scripts/qapi-types.py
> > > @@ -15,6 +15,7 @@ import sys
> > >  import os
> > >  import getopt
> > >  import errno
> > > +import re
> > >  
> > >  def generate_fwd_struct(name, members, builtin_type=False):
> > >      if builtin_type:
> > > @@ -282,9 +283,10 @@ void qapi_free_%(type)s(%(c_type)s obj)
> > >  
> > >  
> > >  try:
> > > -    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> > > +    opts, args = getopt.gnu_getopt(sys.argv[1:], "chbs:p:o:",
> > >                                     ["source", "header", "builtins",
> > > -                                    "prefix=", "output-dir="])
> > > +                                    "schema-dump-file=", "prefix=",
> > > +                                    "output-dir="])
> > >  except getopt.GetoptError, err:
> > >      print str(err)
> > >      sys.exit(1)
> > > @@ -293,6 +295,7 @@ output_dir = ""
> > >  prefix = ""
> > >  c_file = 'qapi-types.c'
> > >  h_file = 'qapi-types.h'
> > > +schema_dump_file = ""
> > >  
> > >  do_c = False
> > >  do_h = False
> > > @@ -309,11 +312,17 @@ for o, a in opts:
> > >          do_h = True
> > >      elif o in ("-b", "--builtins"):
> > >          do_builtins = True
> > > +    elif o in ("-s", "--schema-dump-file"):
> > > +        schema_dump_file = a
> > 
> > Instead of adding this to qapi-types.py, wouldn't it be clearer to add
> > a qapi-dump.py file instead?
> 
> I used scripts/qapi-introspect.py to generate qapi-introspect.h.
> Q: qapi-introspect.py or qapi-dump.py? which one is better?

qapi-introspect.py is good.

> 
> It also helps to extend schema and generate a nested dictionaries with
> metadata, it's very simple to use python to extend.
> 
> /* OrderedDict([('command', 'query-name'), ('returns', 'NameInfo')]) */
>     "{'_obj_member': 'False', '_obj_type': 'command', '_obj_name': 'query-name', '_obj_data': {'returns': {'_obj_type': 'type', '_obj_member': 'False', '_obj_name': 'NameInfo', '_obj_data': {'data': {'_obj_type': 'anonymous-struct', '_obj_member': 'True', '_obj_name': '', '_obj_data': {'*name': 'str'}, '_obj_recursion': 'false'}}, '_obj_recursion': 'true'}}, '_obj_recursion': 'false'}",
> 
> Then in qmp.c, we only need to visit the dictionaries and fill the data
> to allocated structs, which will be returned to QMP monitor.
>  
> > Also, I think it would be interesting to split this patch into two. The first
> > patch changes qapi.py (and related files), this will allow you to better
> > describe your changes to that file. The second patch adds qapi-dump.py.
> > 
> > In general, this looks good to me but I haven't looked into the
> > changes done in qapi.py in detail.
>  
> In v3, we just change qapi.py:parse_schema() to additionally return raw json string.
> In my latest patches, we don't need to change qapi.py, we can directly use OrderedDicts.
> 
> I'm working in qmp.c part, maybe we can try to simple DataObject definitions in V4.
> 
> BTW, no need to review v3, please wait my refreshed V4 :-)
> 
> Thanks, Amos
> 

[Qemu-devel] [PATCH v3 3/3] 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.

It parses all json definition in qapi-schema.json, and generate a
dynamic struct tree, QMP infrastructure will convert the tree to
json string and return to QMP client.

I defined a 'DataObject' union in qapi-schema.json, it's used to
describe the dynamic data struct.

I also added a document about QMP full introspection support
(docs/qmp-full-introspection.txt), it helps to use the new interface
and understand the abstract method in describing the dynamic struct.

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>
---
 docs/qmp-full-introspection.txt |  97 ++++++++++
 qapi-schema.json                | 150 ++++++++++++++++
 qmp-commands.hx                 |  43 ++++-
 qmp.c                           | 382 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 671 insertions(+), 1 deletion(-)
 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..1617df7
--- /dev/null
+++ b/docs/qmp-full-introspection.txt
@@ -0,0 +1,97 @@
+= 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.
+
+== 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 5 seconds to return about 1.5M string.
+
+== '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).
+
+== 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 string to record the visit path, type index of each node
+will be saved to the string, indexes are split by ':'.
+
+Push index to visit_path_str before extending, and pop index from
+visit_path_str after extending.
+
+If the type was already extended in parent node, we don't extend it
+again to avoid dead loop.
+
+'recursive' indicates if the type is extended or not.
diff --git a/qapi-schema.json b/qapi-schema.json
index c3c939c..db500ab 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4235,3 +4235,153 @@
 # Since: 1.7
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
+
+##
+# @DataObjectBase
+#
+# Base of @DataObject
+#
+# @name: #optional @DataObject name
+# @type: @DataObject type
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectBase',
+  'data': { '*name': 'str', 'type': 'str' } }
+
+##
+# @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: type content
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectType',
+  'data': { 'data': [ 'DataObjectMember' ] } }
+
+##
+# @DataObjectUnion
+#
+# Union schema
+#
+# @data: union content
+# @base: union base
+# @discriminator: union discriminator
+#
+# Since: 1.8
+##
+{ 'type': 'DataObjectUnion',
+  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
+            '*discriminator': 'str' } }
+
+##
+# @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
+#
+# Since: 1.8
+##
+{ 'union': 'DataObject',
+  'base': 'DataObjectBase',
+  'discriminator': 'type',
+  'data': {
+    'anonymous-struct': 'DataObjectAnonymousStruct',
+    'command': 'DataObjectCommand',
+    'enumeration': 'DataObjectEnumeration',
+    '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 fba15cd..cf9c4aa 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3237,7 +3237,48 @@ Example:
             "broadcast-allowed": false
         }
       ]
-   }
+
+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
 
diff --git a/qmp.c b/qmp.c
index 1d7a04d..e9bba06 100644
--- a/qmp.c
+++ b/qmp.c
@@ -25,6 +25,8 @@
 #include "sysemu/blockdev.h"
 #include "qom/qom-qobject.h"
 #include "hw/boards.h"
+#include "qmp-schema.h"
+#include "qapi/qmp/qjson.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -486,6 +488,386 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
     return arch_query_cpu_definitions(errp);
 }
 
+/*
+ * use a passed string to record the visit path, schema index of
+ * each extended node will be appended to the string, indexes are
+ * split by ':'
+ */
+static char visit_path_str[1024];
+
+/* push the type index into visit_path_str */
+static void push_id(int id)
+{
+    char *end = strrchr(visit_path_str, ':');
+    char type_idx[256];
+    int num;
+
+    num = sprintf(type_idx, "%d:", id);
+
+    if (end) {
+        /* avoid overflow */
+        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
+        sprintf(end + 1, "%d:", id);
+    } else {
+        sprintf(visit_path_str, "%d:", id);
+    }
+}
+
+/* pop the type index from visit_path_str */
+static void pop_id(void)
+{
+    char *p = strrchr(visit_path_str, ':');
+
+    assert(p != NULL);
+    *p = '\0';
+    p = strrchr(visit_path_str, ':');
+    if (p) {
+        *(p + 1) = '\0';
+    } else {
+        visit_path_str[0] = '\0';
+    }
+}
+
+static const char *qstring_copy_str(QObject *data)
+{
+    QString *qstr;
+
+    if (!data) {
+        return NULL;
+    }
+    qstr = qobject_to_qstring(data);
+    if (qstr) {
+        return qstring_get_str(qstr);
+    } else {
+        return NULL;
+    }
+}
+
+static QObject *get_definition(const char *str, bool update_path)
+{
+    QObject *data, *value;
+    QDict *qdict;
+    int i;
+
+    if (!strcmp(str, "str") || !strcmp(str, "int") ||
+        !strcmp(str, "number") || !strcmp(str, "bool") ||
+        !strcmp(str, "int8") || !strcmp(str, "int16") ||
+        !strcmp(str, "int32") || !strcmp(str, "int64") ||
+        !strcmp(str, "uint8") || !strcmp(str, "uint16") ||
+        !strcmp(str, "uint32") || !strcmp(str, "uint64") ||
+        !strcmp(str, "visitor") || !strcmp(str, "**") ||
+        !strcmp(str, "size")) {
+        return NULL;
+    }
+
+    for (i = 0; qmp_schema_table[i]; i++) {
+        data = qobject_from_json(qmp_schema_table[i]);
+        qdict = qobject_to_qdict(data);
+        assert(qdict != NULL);
+
+        if (qdict_get(qdict, "enum")) {
+            value = qdict_get(qdict, "enum");
+        } else if (qdict_get(qdict, "type")) {
+            value = qdict_get(qdict, "type");
+        } else if (qdict_get(qdict, "union")) {
+            value = qdict_get(qdict, "union");
+        } else {
+            continue;
+        }
+
+        if (!strcmp(str, qstring_copy_str(value)) && update_path) {
+            char *start, *end;
+            char cur_idx[256];
+            char type_idx[256];
+
+            start = visit_path_str;
+            sprintf(type_idx, "%d", i);
+            while (start) {
+                end = strchr(start, ':');
+                if (!end) {
+                    break;
+                }
+                snprintf(cur_idx, end - start + 1, "%s", start);
+                start = end + 1;
+                /* if the type was already extended in parent node,
+                 * we don't extend it again to avoid dead loop. */
+                if (!strcmp(cur_idx, type_idx)) {
+                    return NULL;
+                }
+            }
+            /* push index to visit_path_str before extending */
+            push_id(i);
+        }
+        if (!strcmp(str, qstring_copy_str(value))) {
+
+            return qobject_from_json(qmp_schema_table[i]);
+        }
+    }
+    return NULL;
+}
+
+static DataObject *visit_qobj_dict(QObject *data);
+static DataObject *visit_qobj_list(QObject *data);
+
+/* extend defined type to json object */
+static DataObject *extend_type(const char *str)
+{
+    QObject *data;
+    DataObject *obj;
+
+    data = get_definition(str, true);
+
+    if (data) {
+        obj = visit_qobj_dict(data);
+        pop_id();
+    } else {
+        obj = g_malloc0(sizeof(struct DataObject));
+        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
+        obj->reference_type = g_malloc0(sizeof(String));
+        obj->reference_type->str = g_strdup(str);
+    }
+
+    return obj;
+}
+
+static DataObjectMemberList *list_to_memberlist(QObject *data)
+{
+    DataObjectMemberList *mem_list, *entry, *last_entry;
+    QList *qlist;
+    const QListEntry *lent;
+
+        qlist = qobject_to_qlist(data);
+
+        mem_list = NULL;
+        for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
+            entry = g_malloc0(sizeof(DataObjectMemberList *));
+            entry->value = g_malloc0(sizeof(DataObjectMember));
+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
+
+            if (get_definition(qstring_copy_str(lent->value), false)) {
+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
+                entry->value->has_recursive = true;
+                entry->value->recursive = true;
+                entry->value->type->extend =
+                    extend_type(qstring_copy_str(lent->value));
+            } else {
+                entry->value->type->kind =
+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
+                entry->value->has_recursive = true;
+                entry->value->recursive = false;
+                entry->value->type->reference =
+                    g_strdup(qstring_copy_str(lent->value));
+            }
+
+            entry->next = NULL;
+            if (!mem_list) {
+                mem_list = entry;
+            } else {
+                last_entry->next = entry;
+            }
+            last_entry = entry;
+        }
+        return mem_list;
+}
+
+static DataObjectMemberList *dict_to_memberlist(QObject *data)
+{
+    DataObjectMemberList *mem_list, *entry, *last_entry;
+    QDict *qdict;
+    const QDictEntry *dent;
+
+        qdict = qobject_to_qdict(data);
+
+        mem_list = NULL;
+        for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
+            entry = g_malloc0(sizeof(DataObjectMemberList *));
+            entry->value = g_malloc0(sizeof(DataObjectMember));
+
+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
+
+            if (dent->value->type->code == QTYPE_QDICT) {
+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
+                entry->value->type->extend = visit_qobj_dict(dent->value);
+            } else if (dent->value->type->code == QTYPE_QLIST) {
+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
+                entry->value->type->extend = visit_qobj_list(dent->value);
+            } else if (get_definition(qstring_copy_str(dent->value), false)) {
+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
+                entry->value->has_recursive = true;
+                entry->value->recursive = true;
+                entry->value->type->extend =
+                    extend_type(qstring_copy_str(dent->value));
+            } else {
+                entry->value->type->kind =
+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
+                entry->value->has_recursive = true;
+                entry->value->recursive = false;
+                entry->value->type->reference =
+                    g_strdup(qstring_copy_str(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);
+            }
+
+            entry->next = NULL;
+            if (!mem_list) {
+                mem_list = entry;
+            } else {
+                last_entry->next = entry;
+            }
+            last_entry = entry;
+        }
+        return mem_list;
+}
+
+static DataObject *visit_qobj_list(QObject *data)
+{
+    DataObject *obj;
+
+    obj = g_malloc0(sizeof(struct DataObject));
+    obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
+    obj->anonymous_struct = g_malloc0(sizeof(struct
+                                             DataObjectAnonymousStruct));
+    obj->anonymous_struct->data = list_to_memberlist(data);
+
+    return obj;
+}
+
+static strList *get_str_list(QObject *data)
+{
+    strList *str_list, *last_str_entry, *str_entry;
+    QList *qlist;
+    const QListEntry *lent;
+
+    qlist = qobject_to_qlist(data);
+    str_list = NULL;
+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
+        str_entry = g_malloc0(sizeof(strList *));
+        str_entry->value = g_strdup(qstring_copy_str(lent->value));
+        str_entry->next = NULL;
+        if (!str_list) {
+            str_list = str_entry;
+        } else {
+            last_str_entry->next = str_entry;
+        }
+        last_str_entry = str_entry;
+    }
+
+    return str_list;
+}
+
+static DataObject *visit_qobj_dict(QObject *data)
+{
+    DataObject *obj;
+    QObject *subdata;
+    QDict *qdict;
+
+    qdict = qobject_to_qdict(data);
+    assert(qdict != NULL);
+    obj = g_malloc0(sizeof(*obj));
+
+    if (qdict_get(qdict, "command")) {
+        obj->kind = DATA_OBJECT_KIND_COMMAND;
+        obj->has_name = true;
+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "command")));
+        obj->command = g_malloc0(sizeof(struct DataObjectCommand));
+
+        subdata = qdict_get(qdict, "data");
+        if (subdata && subdata->type->code == QTYPE_QDICT) {
+            obj->command->has_data = true;
+            obj->command->data = dict_to_memberlist(subdata);
+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
+            abort();
+        } else if (subdata) {
+            obj->command->has_data = true;
+            obj->command->data =
+                dict_to_memberlist(get_definition(qstring_copy_str(subdata),
+                                                  true));
+            pop_id();
+        }
+
+        subdata = qdict_get(qdict, "returns");
+        if (subdata && subdata->type->code == QTYPE_QDICT) {
+            abort();
+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
+            obj->command->has_returns = true;
+            obj->command->returns = visit_qobj_list(subdata);
+        } else if (subdata && subdata->type->code == QTYPE_QSTRING) {
+            obj->command->has_returns = true;
+            obj->command->returns = extend_type(qstring_copy_str(subdata));
+        }
+
+        subdata = qdict_get(qdict, "gen");
+        if (subdata && subdata->type->code == QTYPE_QSTRING) {
+            obj->command->has_gen = true;
+            if (!strcmp(qstring_copy_str(subdata), "no")) {
+                obj->command->gen = false;
+            } else {
+                obj->command->gen = true;
+            }
+        }
+    } else if (qdict_get(qdict, "union")) {
+        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
+        obj->has_name = true;
+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "union")));
+        obj->unionobj = g_malloc0(sizeof(struct DataObjectUnion));
+        subdata = qdict_get(qdict, "data");
+        obj->unionobj->data = dict_to_memberlist(subdata);
+    } else if (qdict_get(qdict, "type")) {
+        obj->kind = DATA_OBJECT_KIND_TYPE;
+        obj->has_name = true;
+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "type")));
+        obj->type = g_malloc0(sizeof(struct DataObjectType));
+        subdata = qdict_get(qdict, "data");
+        obj->type->data = dict_to_memberlist(subdata);
+    } else if (qdict_get(qdict, "enum")) {
+        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
+        obj->has_name = true;
+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "enum")));
+        obj->enumeration = g_malloc0(sizeof(struct DataObjectEnumeration));
+        subdata = qdict_get(qdict, "data");
+        obj->enumeration->data = get_str_list(subdata);
+    } else {
+        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
+        obj->anonymous_struct = g_malloc0(sizeof(struct
+                                                 DataObjectAnonymousStruct));
+        obj->anonymous_struct->data = dict_to_memberlist(data);
+    }
+
+    return obj;
+}
+
+DataObjectList *qmp_query_qmp_schema(Error **errp)
+{
+    DataObjectList *list, *last_entry, *entry;
+    QObject *data;
+    int i;
+
+    list = NULL;
+    for (i = 0; qmp_schema_table[i]; i++) {
+        data = qobject_from_json(qmp_schema_table[i]);
+        assert(data != NULL);
+
+        entry = g_malloc0(sizeof(DataObjectList *));
+        memset(visit_path_str, 0, sizeof(visit_path_str));
+        entry->value = visit_qobj_dict(data);
+        entry->next = NULL;
+        if (!list) {
+            list = entry;
+        } else {
+            last_entry->next = entry;
+        }
+        last_entry = entry;
+    }
+
+    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 v3 3/3] qmp: full introspection support for QMP

[Fam Zheng]

On 2014年01月05日 20:02, 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.
>
> It parses all json definition in qapi-schema.json, and generate a
> dynamic struct tree, QMP infrastructure will convert the tree to
> json string and return to QMP client.
>
> I defined a 'DataObject' union in qapi-schema.json, it's used to
> describe the dynamic data struct.
>
> I also added a document about QMP full introspection support
> (docs/qmp-full-introspection.txt), it helps to use the new interface
> and understand the abstract method in describing the dynamic struct.
>
> 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>
> ---

I have a few comments on the current implementation below, there are a 
few things to improve. However I agree to what Eric suggested in reply 
to V2: it may be better to generate most of the response data in python 
code at compile time and simplify the logic in C. Because this 
implementation is slow and it is unnecessary runtime computation. It 
also duplicates much of existing qapi.py logic (data types and other 
semantics parsing).

>   docs/qmp-full-introspection.txt |  97 ++++++++++
>   qapi-schema.json                | 150 ++++++++++++++++
>   qmp-commands.hx                 |  43 ++++-
>   qmp.c                           | 382 ++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 671 insertions(+), 1 deletion(-)
>   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..1617df7
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,97 @@
> += 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.
> +
> +== 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 5 seconds to return about 1.5M string.
> +
> +== '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).
> +
> +== 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 string to record the visit path, type index of each node
> +will be saved to the string, indexes are split by ':'.
> +
> +Push index to visit_path_str before extending, and pop index from
> +visit_path_str after extending.
> +
> +If the type was already extended in parent node, we don't extend it
> +again to avoid dead loop.
> +
> +'recursive' indicates if the type is extended or not.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c3c939c..db500ab 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4235,3 +4235,153 @@
>   # Since: 1.7
>   ##
>   { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> +
> +##
> +# @DataObjectBase
> +#
> +# Base of @DataObject
> +#
> +# @name: #optional @DataObject name
> +# @type: @DataObject type
> +#
> +# Since: 1.8
> +##
> +{ 'type': 'DataObjectBase',
> +  'data': { '*name': 'str', 'type': 'str' } }
> +
> +##
> +# @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: type content
> +#
> +# Since: 1.8
> +##
> +{ 'type': 'DataObjectType',
> +  'data': { 'data': [ 'DataObjectMember' ] } }
> +
> +##
> +# @DataObjectUnion
> +#
> +# Union schema
> +#
> +# @data: union content
> +# @base: union base
> +# @discriminator: union discriminator
> +#
> +# Since: 1.8
> +##
> +{ 'type': 'DataObjectUnion',
> +  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> +            '*discriminator': 'str' } }
> +
> +##
> +# @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
> +#
> +# Since: 1.8
> +##
> +{ 'union': 'DataObject',
> +  'base': 'DataObjectBase',
> +  'discriminator': 'type',
> +  'data': {
> +    'anonymous-struct': 'DataObjectAnonymousStruct',
> +    'command': 'DataObjectCommand',
> +    'enumeration': 'DataObjectEnumeration',
> +    '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 fba15cd..cf9c4aa 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3237,7 +3237,48 @@ Example:
>               "broadcast-allowed": false
>           }
>         ]
> -   }
> +
> +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
>
> diff --git a/qmp.c b/qmp.c
> index 1d7a04d..e9bba06 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -25,6 +25,8 @@
>   #include "sysemu/blockdev.h"
>   #include "qom/qom-qobject.h"
>   #include "hw/boards.h"
> +#include "qmp-schema.h"
> +#include "qapi/qmp/qjson.h"
>
>   NameInfo *qmp_query_name(Error **errp)
>   {
> @@ -486,6 +488,386 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>       return arch_query_cpu_definitions(errp);
>   }
>
> +/*
> + * use a passed string to record the visit path, schema index of
> + * each extended node will be appended to the string, indexes are
> + * split by ':'
> + */
> +static char visit_path_str[1024];

Is it safe to use a global variable here? Is there any race condition 
(i.e. multiple monitor commands run parallel)?

IIUC this serves as a queue of number? Why not use array of int?

> +
> +/* push the type index into visit_path_str */
> +static void push_id(int id)
> +{
> +    char *end = strrchr(visit_path_str, ':');
> +    char type_idx[256];
> +    int num;
> +
> +    num = sprintf(type_idx, "%d:", id);
> +
> +    if (end) {
> +        /* avoid overflow */
> +        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> +        sprintf(end + 1, "%d:", id);
> +    } else {
> +        sprintf(visit_path_str, "%d:", id);
> +    }
> +}
> +
> +/* pop the type index from visit_path_str */
> +static void pop_id(void)
> +{
> +    char *p = strrchr(visit_path_str, ':');
> +
> +    assert(p != NULL);
> +    *p = '\0';
> +    p = strrchr(visit_path_str, ':');
> +    if (p) {
> +        *(p + 1) = '\0';
> +    } else {
> +        visit_path_str[0] = '\0';
> +    }
> +}
> +
> +static const char *qstring_copy_str(QObject *data)

The string is not copied, it's misleading. And I think you could just 
use qstring_get_str in the caller or define a new qstring_ util function 
in qobject/qstring.c.

> +{
> +    QString *qstr;
> +
> +    if (!data) {
> +        return NULL;
> +    }
> +    qstr = qobject_to_qstring(data);
> +    if (qstr) {
> +        return qstring_get_str(qstr);
> +    } else {
> +        return NULL;
> +    }
> +}
> +
> +static QObject *get_definition(const char *str, bool update_path)
> +{
> +    QObject *data, *value;
> +    QDict *qdict;
> +    int i;
> +
> +    if (!strcmp(str, "str") || !strcmp(str, "int") ||
> +        !strcmp(str, "number") || !strcmp(str, "bool") ||
> +        !strcmp(str, "int8") || !strcmp(str, "int16") ||
> +        !strcmp(str, "int32") || !strcmp(str, "int64") ||
> +        !strcmp(str, "uint8") || !strcmp(str, "uint16") ||
> +        !strcmp(str, "uint32") || !strcmp(str, "uint64") ||
> +        !strcmp(str, "visitor") || !strcmp(str, "**") ||
> +        !strcmp(str, "size")) {
> +        return NULL;
> +    }
> +
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        qdict = qobject_to_qdict(data);
> +        assert(qdict != NULL);
> +
> +        if (qdict_get(qdict, "enum")) {
> +            value = qdict_get(qdict, "enum");
> +        } else if (qdict_get(qdict, "type")) {
> +            value = qdict_get(qdict, "type");
> +        } else if (qdict_get(qdict, "union")) {
> +            value = qdict_get(qdict, "union");
> +        } else {
> +            continue;
> +        }
> +
> +        if (!strcmp(str, qstring_copy_str(value)) && update_path) {

Simply:

if (strcmp(str, qstring_copy_str(value)) {
     continue;
}

Then avoid another check for this.

> +            char *start, *end;
> +            char cur_idx[256];
> +            char type_idx[256];
> +
> +            start = visit_path_str;
> +            sprintf(type_idx, "%d", i);

Let's avoid sprintf completely.

> +            while (start) {
> +                end = strchr(start, ':');
> +                if (!end) {
> +                    break;
> +                }
> +                snprintf(cur_idx, end - start + 1, "%s", start);
> +                start = end + 1;
> +                /* if the type was already extended in parent node,
> +                 * we don't extend it again to avoid dead loop. */
> +                if (!strcmp(cur_idx, type_idx)) {
> +                    return NULL;
> +                }
> +            }

Using array of int will get you a simpler code here.

Furthermore, you could avoid linear lookup by using a bitmap beside 
visit_path_str to keep track of what is visited.

> +            /* push index to visit_path_str before extending */
> +            push_id(i);
> +        }
> +        if (!strcmp(str, qstring_copy_str(value))) {
> +
> +            return qobject_from_json(qmp_schema_table[i]);
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static DataObject *visit_qobj_dict(QObject *data);
> +static DataObject *visit_qobj_list(QObject *data);
> +
> +/* extend defined type to json object */
> +static DataObject *extend_type(const char *str)
> +{
> +    QObject *data;
> +    DataObject *obj;
> +
> +    data = get_definition(str, true);
> +
> +    if (data) {
> +        obj = visit_qobj_dict(data);
> +        pop_id();
> +    } else {
> +        obj = g_malloc0(sizeof(struct DataObject));
> +        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> +        obj->reference_type = g_malloc0(sizeof(String));
> +        obj->reference_type->str = g_strdup(str);
> +    }
> +
> +    return obj;
> +}
> +
> +static DataObjectMemberList *list_to_memberlist(QObject *data)
> +{
> +    DataObjectMemberList *mem_list, *entry, *last_entry;
> +    QList *qlist;
> +    const QListEntry *lent;
> +
> +        qlist = qobject_to_qlist(data);

Bad indentation starting from here.

> +
> +        mem_list = NULL;
> +        for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> +            entry = g_malloc0(sizeof(DataObjectMemberList *));

Pointer size allocated instead of structure.

> +            entry->value = g_malloc0(sizeof(DataObjectMember));
> +            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> +
> +            if (get_definition(qstring_copy_str(lent->value), false)) {
> +                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> +                entry->value->has_recursive = true;
> +                entry->value->recursive = true;
> +                entry->value->type->extend =
> +                    extend_type(qstring_copy_str(lent->value));
> +            } else {
> +                entry->value->type->kind =
> +                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> +                entry->value->has_recursive = true;
> +                entry->value->recursive = false;
> +                entry->value->type->reference =
> +                    g_strdup(qstring_copy_str(lent->value));
> +            }
> +
> +            entry->next = NULL;
> +            if (!mem_list) {
> +                mem_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +        return mem_list;
> +}
> +
> +static DataObjectMemberList *dict_to_memberlist(QObject *data)
> +{
> +    DataObjectMemberList *mem_list, *entry, *last_entry;
> +    QDict *qdict;
> +    const QDictEntry *dent;
> +
> +        qdict = qobject_to_qdict(data);
> +
> +        mem_list = NULL;
> +        for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> +            entry = g_malloc0(sizeof(DataObjectMemberList *));

Same here.

> +            entry->value = g_malloc0(sizeof(DataObjectMember));
> +
> +            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> +
> +            if (dent->value->type->code == QTYPE_QDICT) {
> +                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> +                entry->value->type->extend = visit_qobj_dict(dent->value);
> +            } else if (dent->value->type->code == QTYPE_QLIST) {
> +                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> +                entry->value->type->extend = visit_qobj_list(dent->value);
> +            } else if (get_definition(qstring_copy_str(dent->value), false)) {
> +                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> +                entry->value->has_recursive = true;
> +                entry->value->recursive = true;
> +                entry->value->type->extend =
> +                    extend_type(qstring_copy_str(dent->value));
> +            } else {
> +                entry->value->type->kind =
> +                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> +                entry->value->has_recursive = true;
> +                entry->value->recursive = false;
> +                entry->value->type->reference =
> +                    g_strdup(qstring_copy_str(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);
> +            }
> +
> +            entry->next = NULL;
> +            if (!mem_list) {
> +                mem_list = entry;
> +            } else {
> +                last_entry->next = entry;
> +            }
> +            last_entry = entry;
> +        }
> +        return mem_list;
> +}
> +
> +static DataObject *visit_qobj_list(QObject *data)
> +{
> +    DataObject *obj;
> +
> +    obj = g_malloc0(sizeof(struct DataObject));
> +    obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> +    obj->anonymous_struct = g_malloc0(sizeof(struct
> +                                             DataObjectAnonymousStruct));
> +    obj->anonymous_struct->data = list_to_memberlist(data);
> +
> +    return obj;
> +}
> +
> +static strList *get_str_list(QObject *data)
> +{
> +    strList *str_list, *last_str_entry, *str_entry;
> +    QList *qlist;
> +    const QListEntry *lent;
> +
> +    qlist = qobject_to_qlist(data);
> +    str_list = NULL;
> +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> +        str_entry = g_malloc0(sizeof(strList *));

And here.

> +        str_entry->value = g_strdup(qstring_copy_str(lent->value));
> +        str_entry->next = NULL;
> +        if (!str_list) {
> +            str_list = str_entry;
> +        } else {
> +            last_str_entry->next = str_entry;
> +        }
> +        last_str_entry = str_entry;
> +    }
> +
> +    return str_list;
> +}
> +
> +static DataObject *visit_qobj_dict(QObject *data)
> +{
> +    DataObject *obj;
> +    QObject *subdata;
> +    QDict *qdict;
> +
> +    qdict = qobject_to_qdict(data);
> +    assert(qdict != NULL);
> +    obj = g_malloc0(sizeof(*obj));
> +
> +    if (qdict_get(qdict, "command")) {
> +        obj->kind = DATA_OBJECT_KIND_COMMAND;
> +        obj->has_name = true;
> +        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "command")));
> +        obj->command = g_malloc0(sizeof(struct DataObjectCommand));
> +
> +        subdata = qdict_get(qdict, "data");
> +        if (subdata && subdata->type->code == QTYPE_QDICT) {
> +            obj->command->has_data = true;
> +            obj->command->data = dict_to_memberlist(subdata);
> +        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> +            abort();
> +        } else if (subdata) {
> +            obj->command->has_data = true;
> +            obj->command->data =
> +                dict_to_memberlist(get_definition(qstring_copy_str(subdata),
> +                                                  true));
> +            pop_id();
> +        }
> +
> +        subdata = qdict_get(qdict, "returns");
> +        if (subdata && subdata->type->code == QTYPE_QDICT) {
> +            abort();
> +        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> +            obj->command->has_returns = true;
> +            obj->command->returns = visit_qobj_list(subdata);
> +        } else if (subdata && subdata->type->code == QTYPE_QSTRING) {
> +            obj->command->has_returns = true;
> +            obj->command->returns = extend_type(qstring_copy_str(subdata));
> +        }
> +
> +        subdata = qdict_get(qdict, "gen");
> +        if (subdata && subdata->type->code == QTYPE_QSTRING) {
> +            obj->command->has_gen = true;
> +            if (!strcmp(qstring_copy_str(subdata), "no")) {
> +                obj->command->gen = false;
> +            } else {
> +                obj->command->gen = true;
> +            }
> +        }
> +    } else if (qdict_get(qdict, "union")) {
> +        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> +        obj->has_name = true;
> +        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "union")));
> +        obj->unionobj = g_malloc0(sizeof(struct DataObjectUnion));
> +        subdata = qdict_get(qdict, "data");
> +        obj->unionobj->data = dict_to_memberlist(subdata);
> +    } else if (qdict_get(qdict, "type")) {
> +        obj->kind = DATA_OBJECT_KIND_TYPE;
> +        obj->has_name = true;
> +        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "type")));
> +        obj->type = g_malloc0(sizeof(struct DataObjectType));
> +        subdata = qdict_get(qdict, "data");
> +        obj->type->data = dict_to_memberlist(subdata);
> +    } else if (qdict_get(qdict, "enum")) {
> +        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> +        obj->has_name = true;
> +        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "enum")));
> +        obj->enumeration = g_malloc0(sizeof(struct DataObjectEnumeration));
> +        subdata = qdict_get(qdict, "data");
> +        obj->enumeration->data = get_str_list(subdata);
> +    } else {
> +        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> +        obj->anonymous_struct = g_malloc0(sizeof(struct
> +                                                 DataObjectAnonymousStruct));
> +        obj->anonymous_struct->data = dict_to_memberlist(data);
> +    }
> +
> +    return obj;
> +}
> +
> +DataObjectList *qmp_query_qmp_schema(Error **errp)
> +{
> +    DataObjectList *list, *last_entry, *entry;
> +    QObject *data;
> +    int i;
> +
> +    list = NULL;
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        assert(data != NULL);
> +
> +        entry = g_malloc0(sizeof(DataObjectList *));

And here.

> +        memset(visit_path_str, 0, sizeof(visit_path_str));
> +        entry->value = visit_qobj_dict(data);
> +        entry->next = NULL;
> +        if (!list) {

Why not use a pointer to pointer to avoid this if branch?

> +            list = entry;
> +        } else {
> +            last_entry->next = entry;
> +        }
> +        last_entry = entry;
> +    }
> +
> +    return list;
> +}
> +
>   void qmp_add_client(const char *protocol, const char *fdname,
>                       bool has_skipauth, bool skipauth, bool has_tls, bool tls,
>                       Error **errp)
>

Fam

Re: [Qemu-devel] [PATCH v3 3/3] qmp: full introspection support for QMP

[Amos Kong]

On Mon, Jan 06, 2014 at 05:37:19PM +0800, Fam Zheng wrote:
> On 2014年01月05日 20:02, 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.
> >
> >It parses all json definition in qapi-schema.json, and generate a
> >dynamic struct tree, QMP infrastructure will convert the tree to
> >json string and return to QMP client.
> >
> >I defined a 'DataObject' union in qapi-schema.json, it's used to
> >describe the dynamic data struct.
> >
> >I also added a document about QMP full introspection support
> >(docs/qmp-full-introspection.txt), it helps to use the new interface
> >and understand the abstract method in describing the dynamic struct.
> >
> >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>
> >---

Hi Fam,
 
I'm very happy to get your comments!

> I have a few comments on the current implementation below, there are
> a few things to improve. However I agree to what Eric suggested in
> reply to V2: it may be better to generate most of the response data
> in python code at compile time and simplify the logic in C. Because
> this implementation is slow and it is unnecessary runtime
> computation. It also duplicates much of existing qapi.py logic (data
> types and other semantics parsing).

The implemented code of QMP command will generate defined struct,
the memory is allocated in run time, it will be auto released by
QMP infrastructure.

We can implement the parse/extend work by Python and generate
a result in a head file, that can be directly & easyly used C code.
Maybe we can generate a nested & complex struct definition in the
head file, and use less g_malloc() to generate return struct.
I will try, thanks.
 
> >  docs/qmp-full-introspection.txt |  97 ++++++++++
> >  qapi-schema.json                | 150 ++++++++++++++++
> >  qmp-commands.hx                 |  43 ++++-
> >  qmp.c                           | 382 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 671 insertions(+), 1 deletion(-)
> >  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..1617df7
> >--- /dev/null
> >+++ b/docs/qmp-full-introspection.txt
> >@@ -0,0 +1,97 @@
> >+= 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.
> >+
> >+== 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 5 seconds to return about 1.5M string.
> >+
> >+== '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).
> >+
> >+== 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 string to record the visit path, type index of each node
> >+will be saved to the string, indexes are split by ':'.
> >+
> >+Push index to visit_path_str before extending, and pop index from
> >+visit_path_str after extending.
> >+
> >+If the type was already extended in parent node, we don't extend it
> >+again to avoid dead loop.
> >+
> >+'recursive' indicates if the type is extended or not.
> >diff --git a/qapi-schema.json b/qapi-schema.json
> >index c3c939c..db500ab 100644
> >--- a/qapi-schema.json
> >+++ b/qapi-schema.json
> >@@ -4235,3 +4235,153 @@
> >  # Since: 1.7
> >  ##
> >  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> >+
> >+##
> >+# @DataObjectBase
> >+#
> >+# Base of @DataObject
> >+#
> >+# @name: #optional @DataObject name
> >+# @type: @DataObject type
> >+#
> >+# Since: 1.8
> >+##
> >+{ 'type': 'DataObjectBase',
> >+  'data': { '*name': 'str', 'type': 'str' } }
> >+
> >+##
> >+# @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: type content
> >+#
> >+# Since: 1.8
> >+##
> >+{ 'type': 'DataObjectType',
> >+  'data': { 'data': [ 'DataObjectMember' ] } }
> >+
> >+##
> >+# @DataObjectUnion
> >+#
> >+# Union schema
> >+#
> >+# @data: union content
> >+# @base: union base
> >+# @discriminator: union discriminator
> >+#
> >+# Since: 1.8
> >+##
> >+{ 'type': 'DataObjectUnion',
> >+  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> >+            '*discriminator': 'str' } }
> >+
> >+##
> >+# @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
> >+#
> >+# Since: 1.8
> >+##
> >+{ 'union': 'DataObject',
> >+  'base': 'DataObjectBase',
> >+  'discriminator': 'type',
> >+  'data': {
> >+    'anonymous-struct': 'DataObjectAnonymousStruct',
> >+    'command': 'DataObjectCommand',
> >+    'enumeration': 'DataObjectEnumeration',
> >+    '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 fba15cd..cf9c4aa 100644
> >--- a/qmp-commands.hx
> >+++ b/qmp-commands.hx
> >@@ -3237,7 +3237,48 @@ Example:
> >              "broadcast-allowed": false
> >          }
> >        ]
> >-   }
> >+
> >+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
> >
> >diff --git a/qmp.c b/qmp.c
> >index 1d7a04d..e9bba06 100644
> >--- a/qmp.c
> >+++ b/qmp.c
> >@@ -25,6 +25,8 @@
> >  #include "sysemu/blockdev.h"
> >  #include "qom/qom-qobject.h"
> >  #include "hw/boards.h"
> >+#include "qmp-schema.h"
> >+#include "qapi/qmp/qjson.h"
> >
> >  NameInfo *qmp_query_name(Error **errp)
> >  {
> >@@ -486,6 +488,386 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> >      return arch_query_cpu_definitions(errp);
> >  }
> >
> >+/*
> >+ * use a passed string to record the visit path, schema index of
> >+ * each extended node will be appended to the string, indexes are
> >+ * split by ':'
> >+ */
> >+static char visit_path_str[1024];
> 
> Is it safe to use a global variable here? Is there any race
> condition (i.e. multiple monitor commands run parallel)?

This command is only executed once after installing/updating qemu
for Libvirt. But the problem you pointed is existed. I will allocate
/free dynamic memory for each query instance.
 
> IIUC this serves as a queue of number? Why not use array of int?
 
Great! It's easier to understand and update.

> >+
> >+/* push the type index into visit_path_str */
> >+static void push_id(int id)
> >+{
> >+    char *end = strrchr(visit_path_str, ':');
> >+    char type_idx[256];
> >+    int num;
> >+
> >+    num = sprintf(type_idx, "%d:", id);
> >+
> >+    if (end) {
> >+        /* avoid overflow */
> >+        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> >+        sprintf(end + 1, "%d:", id);
> >+    } else {
> >+        sprintf(visit_path_str, "%d:", id);
> >+    }
> >+}
> >+
> >+/* pop the type index from visit_path_str */
> >+static void pop_id(void)
> >+{
> >+    char *p = strrchr(visit_path_str, ':');
> >+
> >+    assert(p != NULL);
> >+    *p = '\0';
> >+    p = strrchr(visit_path_str, ':');
> >+    if (p) {
> >+        *(p + 1) = '\0';
> >+    } else {
> >+        visit_path_str[0] = '\0';
> >+    }
> >+}
> >+
> >+static const char *qstring_copy_str(QObject *data)
> 
> The string is not copied, it's misleading. And I think you could
> just use qstring_get_str in the caller or define a new qstring_ util
> function in qobject/qstring.c.

OK.

> 
> >+{
> >+    QString *qstr;
> >+
> >+    if (!data) {
> >+        return NULL;
> >+    }
> >+    qstr = qobject_to_qstring(data);
> >+    if (qstr) {
> >+        return qstring_get_str(qstr);
> >+    } else {
> >+        return NULL;
> >+    }
> >+}
> >+
> >+static QObject *get_definition(const char *str, bool update_path)
> >+{
> >+    QObject *data, *value;
> >+    QDict *qdict;
> >+    int i;
> >+
> >+    if (!strcmp(str, "str") || !strcmp(str, "int") ||
> >+        !strcmp(str, "number") || !strcmp(str, "bool") ||
> >+        !strcmp(str, "int8") || !strcmp(str, "int16") ||
> >+        !strcmp(str, "int32") || !strcmp(str, "int64") ||
> >+        !strcmp(str, "uint8") || !strcmp(str, "uint16") ||
> >+        !strcmp(str, "uint32") || !strcmp(str, "uint64") ||
> >+        !strcmp(str, "visitor") || !strcmp(str, "**") ||
> >+        !strcmp(str, "size")) {
> >+        return NULL;
> >+    }
> >+
> >+    for (i = 0; qmp_schema_table[i]; i++) {
> >+        data = qobject_from_json(qmp_schema_table[i]);
> >+        qdict = qobject_to_qdict(data);
> >+        assert(qdict != NULL);
> >+
> >+        if (qdict_get(qdict, "enum")) {
> >+            value = qdict_get(qdict, "enum");
> >+        } else if (qdict_get(qdict, "type")) {
> >+            value = qdict_get(qdict, "type");
> >+        } else if (qdict_get(qdict, "union")) {
> >+            value = qdict_get(qdict, "union");
> >+        } else {
> >+            continue;
> >+        }
> >+
> >+        if (!strcmp(str, qstring_copy_str(value)) && update_path) {
> 
> Simply:
> 
> if (strcmp(str, qstring_copy_str(value)) {
>     continue;
> }
> 
> Then avoid another check for this.
> 
> >+            char *start, *end;
> >+            char cur_idx[256];
> >+            char type_idx[256];
> >+
> >+            start = visit_path_str;
> >+            sprintf(type_idx, "%d", i);
> 
> Let's avoid sprintf completely.
> 
> >+            while (start) {
> >+                end = strchr(start, ':');
> >+                if (!end) {
> >+                    break;
> >+                }
> >+                snprintf(cur_idx, end - start + 1, "%s", start);
> >+                start = end + 1;
> >+                /* if the type was already extended in parent node,
> >+                 * we don't extend it again to avoid dead loop. */
> >+                if (!strcmp(cur_idx, type_idx)) {
> >+                    return NULL;
> >+                }
> >+            }
> 
> Using array of int will get you a simpler code here.
> 
> Furthermore, you could avoid linear lookup by using a bitmap beside
> visit_path_str to keep track of what is visited.
> 
> >+            /* push index to visit_path_str before extending */
> >+            push_id(i);
> >+        }
> >+        if (!strcmp(str, qstring_copy_str(value))) {
> >+
> >+            return qobject_from_json(qmp_schema_table[i]);
> >+        }
> >+    }
> >+    return NULL;
> >+}
> >+
> >+static DataObject *visit_qobj_dict(QObject *data);
> >+static DataObject *visit_qobj_list(QObject *data);
> >+
> >+/* extend defined type to json object */
> >+static DataObject *extend_type(const char *str)
> >+{
> >+    QObject *data;
> >+    DataObject *obj;
> >+
> >+    data = get_definition(str, true);
> >+
> >+    if (data) {
> >+        obj = visit_qobj_dict(data);
> >+        pop_id();
> >+    } else {
> >+        obj = g_malloc0(sizeof(struct DataObject));
> >+        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> >+        obj->reference_type = g_malloc0(sizeof(String));
> >+        obj->reference_type->str = g_strdup(str);
> >+    }
> >+
> >+    return obj;
> >+}
> >+
> >+static DataObjectMemberList *list_to_memberlist(QObject *data)
> >+{
> >+    DataObjectMemberList *mem_list, *entry, *last_entry;
> >+    QList *qlist;
> >+    const QListEntry *lent;
> >+
> >+        qlist = qobject_to_qlist(data);
> 
> Bad indentation starting from here.
> 
> >+
> >+        mem_list = NULL;
> >+        for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >+            entry = g_malloc0(sizeof(DataObjectMemberList *));
> 
> Pointer size allocated instead of structure.
> 
> >+            entry->value = g_malloc0(sizeof(DataObjectMember));
> >+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> >+
> >+            if (get_definition(qstring_copy_str(lent->value), false)) {
> >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >+                entry->value->has_recursive = true;
> >+                entry->value->recursive = true;
> >+                entry->value->type->extend =
> >+                    extend_type(qstring_copy_str(lent->value));
> >+            } else {
> >+                entry->value->type->kind =
> >+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> >+                entry->value->has_recursive = true;
> >+                entry->value->recursive = false;
> >+                entry->value->type->reference =
> >+                    g_strdup(qstring_copy_str(lent->value));
> >+            }
> >+
> >+            entry->next = NULL;
> >+            if (!mem_list) {
> >+                mem_list = entry;
> >+            } else {
> >+                last_entry->next = entry;
> >+            }
> >+            last_entry = entry;
> >+        }
> >+        return mem_list;
> >+}
> >+
> >+static DataObjectMemberList *dict_to_memberlist(QObject *data)
> >+{
> >+    DataObjectMemberList *mem_list, *entry, *last_entry;
> >+    QDict *qdict;
> >+    const QDictEntry *dent;
> >+
> >+        qdict = qobject_to_qdict(data);
> >+
> >+        mem_list = NULL;
> >+        for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> >+            entry = g_malloc0(sizeof(DataObjectMemberList *));
> 
> Same here.
> 
> >+            entry->value = g_malloc0(sizeof(DataObjectMember));
> >+
> >+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> >+
> >+            if (dent->value->type->code == QTYPE_QDICT) {
> >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >+                entry->value->type->extend = visit_qobj_dict(dent->value);
> >+            } else if (dent->value->type->code == QTYPE_QLIST) {
> >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >+                entry->value->type->extend = visit_qobj_list(dent->value);
> >+            } else if (get_definition(qstring_copy_str(dent->value), false)) {
> >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >+                entry->value->has_recursive = true;
> >+                entry->value->recursive = true;
> >+                entry->value->type->extend =
> >+                    extend_type(qstring_copy_str(dent->value));
> >+            } else {
> >+                entry->value->type->kind =
> >+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> >+                entry->value->has_recursive = true;
> >+                entry->value->recursive = false;
> >+                entry->value->type->reference =
> >+                    g_strdup(qstring_copy_str(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);
> >+            }
> >+
> >+            entry->next = NULL;
> >+            if (!mem_list) {
> >+                mem_list = entry;
> >+            } else {
> >+                last_entry->next = entry;
> >+            }
> >+            last_entry = entry;
> >+        }
> >+        return mem_list;
> >+}
> >+
> >+static DataObject *visit_qobj_list(QObject *data)
> >+{
> >+    DataObject *obj;
> >+
> >+    obj = g_malloc0(sizeof(struct DataObject));
> >+    obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> >+    obj->anonymous_struct = g_malloc0(sizeof(struct
> >+                                             DataObjectAnonymousStruct));
> >+    obj->anonymous_struct->data = list_to_memberlist(data);
> >+
> >+    return obj;
> >+}
> >+
> >+static strList *get_str_list(QObject *data)
> >+{
> >+    strList *str_list, *last_str_entry, *str_entry;
> >+    QList *qlist;
> >+    const QListEntry *lent;
> >+
> >+    qlist = qobject_to_qlist(data);
> >+    str_list = NULL;
> >+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >+        str_entry = g_malloc0(sizeof(strList *));
> 
> And here.
> 
> >+        str_entry->value = g_strdup(qstring_copy_str(lent->value));
> >+        str_entry->next = NULL;
> >+        if (!str_list) {
> >+            str_list = str_entry;
> >+        } else {
> >+            last_str_entry->next = str_entry;
> >+        }
> >+        last_str_entry = str_entry;
> >+    }
> >+
> >+    return str_list;
> >+}
> >+
> >+static DataObject *visit_qobj_dict(QObject *data)
> >+{
> >+    DataObject *obj;
> >+    QObject *subdata;
> >+    QDict *qdict;
> >+
> >+    qdict = qobject_to_qdict(data);
> >+    assert(qdict != NULL);
> >+    obj = g_malloc0(sizeof(*obj));
> >+
> >+    if (qdict_get(qdict, "command")) {
> >+        obj->kind = DATA_OBJECT_KIND_COMMAND;
> >+        obj->has_name = true;
> >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "command")));
> >+        obj->command = g_malloc0(sizeof(struct DataObjectCommand));
> >+
> >+        subdata = qdict_get(qdict, "data");
> >+        if (subdata && subdata->type->code == QTYPE_QDICT) {
> >+            obj->command->has_data = true;
> >+            obj->command->data = dict_to_memberlist(subdata);
> >+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> >+            abort();
> >+        } else if (subdata) {
> >+            obj->command->has_data = true;
> >+            obj->command->data =
> >+                dict_to_memberlist(get_definition(qstring_copy_str(subdata),
> >+                                                  true));
> >+            pop_id();
> >+        }
> >+
> >+        subdata = qdict_get(qdict, "returns");
> >+        if (subdata && subdata->type->code == QTYPE_QDICT) {
> >+            abort();
> >+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> >+            obj->command->has_returns = true;
> >+            obj->command->returns = visit_qobj_list(subdata);
> >+        } else if (subdata && subdata->type->code == QTYPE_QSTRING) {
> >+            obj->command->has_returns = true;
> >+            obj->command->returns = extend_type(qstring_copy_str(subdata));
> >+        }
> >+
> >+        subdata = qdict_get(qdict, "gen");
> >+        if (subdata && subdata->type->code == QTYPE_QSTRING) {
> >+            obj->command->has_gen = true;
> >+            if (!strcmp(qstring_copy_str(subdata), "no")) {
> >+                obj->command->gen = false;
> >+            } else {
> >+                obj->command->gen = true;
> >+            }
> >+        }
> >+    } else if (qdict_get(qdict, "union")) {
> >+        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> >+        obj->has_name = true;
> >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "union")));
> >+        obj->unionobj = g_malloc0(sizeof(struct DataObjectUnion));
> >+        subdata = qdict_get(qdict, "data");
> >+        obj->unionobj->data = dict_to_memberlist(subdata);
> >+    } else if (qdict_get(qdict, "type")) {
> >+        obj->kind = DATA_OBJECT_KIND_TYPE;
> >+        obj->has_name = true;
> >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "type")));
> >+        obj->type = g_malloc0(sizeof(struct DataObjectType));
> >+        subdata = qdict_get(qdict, "data");
> >+        obj->type->data = dict_to_memberlist(subdata);
> >+    } else if (qdict_get(qdict, "enum")) {
> >+        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> >+        obj->has_name = true;
> >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "enum")));
> >+        obj->enumeration = g_malloc0(sizeof(struct DataObjectEnumeration));
> >+        subdata = qdict_get(qdict, "data");
> >+        obj->enumeration->data = get_str_list(subdata);
> >+    } else {
> >+        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> >+        obj->anonymous_struct = g_malloc0(sizeof(struct
> >+                                                 DataObjectAnonymousStruct));
> >+        obj->anonymous_struct->data = dict_to_memberlist(data);
> >+    }
> >+
> >+    return obj;
> >+}
> >+
> >+DataObjectList *qmp_query_qmp_schema(Error **errp)
> >+{
> >+    DataObjectList *list, *last_entry, *entry;
> >+    QObject *data;
> >+    int i;
> >+
> >+    list = NULL;
> >+    for (i = 0; qmp_schema_table[i]; i++) {
> >+        data = qobject_from_json(qmp_schema_table[i]);
> >+        assert(data != NULL);
> >+
> >+        entry = g_malloc0(sizeof(DataObjectList *));
> 
> And here.
> 
> >+        memset(visit_path_str, 0, sizeof(visit_path_str));
> >+        entry->value = visit_qobj_dict(data);
> >+        entry->next = NULL;
> >+        if (!list) {
> 
> Why not use a pointer to pointer to avoid this if branch?
 
Will fix it.

> >+            list = entry;
> >+        } else {
> >+            last_entry->next = entry;
> >+        }
> >+        last_entry = entry;
> >+    }
> >+
> >+    return list;
> >+}
> >+
> >  void qmp_add_client(const char *protocol, const char *fdname,
> >                      bool has_skipauth, bool skipauth, bool has_tls, bool tls,
> >                      Error **errp)
> >
> 
> Fam

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 3/3] qmp: full introspection support for QMP

[Luiz Capitulino]

On Thu, 9 Jan 2014 17:49:56 +0800
Amos Kong <akong@redhat.com> wrote:

> On Mon, Jan 06, 2014 at 05:37:19PM +0800, Fam Zheng wrote:
> > On 2014年01月05日 20:02, 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.
> > >
> > >It parses all json definition in qapi-schema.json, and generate a
> > >dynamic struct tree, QMP infrastructure will convert the tree to
> > >json string and return to QMP client.
> > >
> > >I defined a 'DataObject' union in qapi-schema.json, it's used to
> > >describe the dynamic data struct.
> > >
> > >I also added a document about QMP full introspection support
> > >(docs/qmp-full-introspection.txt), it helps to use the new interface
> > >and understand the abstract method in describing the dynamic struct.
> > >
> > >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>
> > >---
> 
> Hi Fam,
>  
> I'm very happy to get your comments!
> 
> > I have a few comments on the current implementation below, there are
> > a few things to improve. However I agree to what Eric suggested in
> > reply to V2: it may be better to generate most of the response data
> > in python code at compile time and simplify the logic in C. Because
> > this implementation is slow and it is unnecessary runtime
> > computation. It also duplicates much of existing qapi.py logic (data
> > types and other semantics parsing).
> 
> The implemented code of QMP command will generate defined struct,
> the memory is allocated in run time, it will be auto released by
> QMP infrastructure.
> 
> We can implement the parse/extend work by Python and generate
> a result in a head file, that can be directly & easyly used C code.
> Maybe we can generate a nested & complex struct definition in the
> head file, and use less g_malloc() to generate return struct.
> I will try, thanks.

Which means this patch will completely change, right? In this case
I'm skipping review.

>  
> > >  docs/qmp-full-introspection.txt |  97 ++++++++++
> > >  qapi-schema.json                | 150 ++++++++++++++++
> > >  qmp-commands.hx                 |  43 ++++-
> > >  qmp.c                           | 382 ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 671 insertions(+), 1 deletion(-)
> > >  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..1617df7
> > >--- /dev/null
> > >+++ b/docs/qmp-full-introspection.txt
> > >@@ -0,0 +1,97 @@
> > >+= 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.
> > >+
> > >+== 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 5 seconds to return about 1.5M string.
> > >+
> > >+== '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).
> > >+
> > >+== 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 string to record the visit path, type index of each node
> > >+will be saved to the string, indexes are split by ':'.
> > >+
> > >+Push index to visit_path_str before extending, and pop index from
> > >+visit_path_str after extending.
> > >+
> > >+If the type was already extended in parent node, we don't extend it
> > >+again to avoid dead loop.
> > >+
> > >+'recursive' indicates if the type is extended or not.
> > >diff --git a/qapi-schema.json b/qapi-schema.json
> > >index c3c939c..db500ab 100644
> > >--- a/qapi-schema.json
> > >+++ b/qapi-schema.json
> > >@@ -4235,3 +4235,153 @@
> > >  # Since: 1.7
> > >  ##
> > >  { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > >+
> > >+##
> > >+# @DataObjectBase
> > >+#
> > >+# Base of @DataObject
> > >+#
> > >+# @name: #optional @DataObject name
> > >+# @type: @DataObject type
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectBase',
> > >+  'data': { '*name': 'str', 'type': 'str' } }
> > >+
> > >+##
> > >+# @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: type content
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectType',
> > >+  'data': { 'data': [ 'DataObjectMember' ] } }
> > >+
> > >+##
> > >+# @DataObjectUnion
> > >+#
> > >+# Union schema
> > >+#
> > >+# @data: union content
> > >+# @base: union base
> > >+# @discriminator: union discriminator
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectUnion',
> > >+  'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> > >+            '*discriminator': 'str' } }
> > >+
> > >+##
> > >+# @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
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'union': 'DataObject',
> > >+  'base': 'DataObjectBase',
> > >+  'discriminator': 'type',
> > >+  'data': {
> > >+    'anonymous-struct': 'DataObjectAnonymousStruct',
> > >+    'command': 'DataObjectCommand',
> > >+    'enumeration': 'DataObjectEnumeration',
> > >+    '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 fba15cd..cf9c4aa 100644
> > >--- a/qmp-commands.hx
> > >+++ b/qmp-commands.hx
> > >@@ -3237,7 +3237,48 @@ Example:
> > >              "broadcast-allowed": false
> > >          }
> > >        ]
> > >-   }
> > >+
> > >+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
> > >
> > >diff --git a/qmp.c b/qmp.c
> > >index 1d7a04d..e9bba06 100644
> > >--- a/qmp.c
> > >+++ b/qmp.c
> > >@@ -25,6 +25,8 @@
> > >  #include "sysemu/blockdev.h"
> > >  #include "qom/qom-qobject.h"
> > >  #include "hw/boards.h"
> > >+#include "qmp-schema.h"
> > >+#include "qapi/qmp/qjson.h"
> > >
> > >  NameInfo *qmp_query_name(Error **errp)
> > >  {
> > >@@ -486,6 +488,386 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> > >      return arch_query_cpu_definitions(errp);
> > >  }
> > >
> > >+/*
> > >+ * use a passed string to record the visit path, schema index of
> > >+ * each extended node will be appended to the string, indexes are
> > >+ * split by ':'
> > >+ */
> > >+static char visit_path_str[1024];
> > 
> > Is it safe to use a global variable here? Is there any race
> > condition (i.e. multiple monitor commands run parallel)?
> 
> This command is only executed once after installing/updating qemu
> for Libvirt. But the problem you pointed is existed. I will allocate
> /free dynamic memory for each query instance.
>  
> > IIUC this serves as a queue of number? Why not use array of int?
>  
> Great! It's easier to understand and update.
> 
> > >+
> > >+/* push the type index into visit_path_str */
> > >+static void push_id(int id)
> > >+{
> > >+    char *end = strrchr(visit_path_str, ':');
> > >+    char type_idx[256];
> > >+    int num;
> > >+
> > >+    num = sprintf(type_idx, "%d:", id);
> > >+
> > >+    if (end) {
> > >+        /* avoid overflow */
> > >+        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> > >+        sprintf(end + 1, "%d:", id);
> > >+    } else {
> > >+        sprintf(visit_path_str, "%d:", id);
> > >+    }
> > >+}
> > >+
> > >+/* pop the type index from visit_path_str */
> > >+static void pop_id(void)
> > >+{
> > >+    char *p = strrchr(visit_path_str, ':');
> > >+
> > >+    assert(p != NULL);
> > >+    *p = '\0';
> > >+    p = strrchr(visit_path_str, ':');
> > >+    if (p) {
> > >+        *(p + 1) = '\0';
> > >+    } else {
> > >+        visit_path_str[0] = '\0';
> > >+    }
> > >+}
> > >+
> > >+static const char *qstring_copy_str(QObject *data)
> > 
> > The string is not copied, it's misleading. And I think you could
> > just use qstring_get_str in the caller or define a new qstring_ util
> > function in qobject/qstring.c.
> 
> OK.
> 
> > 
> > >+{
> > >+    QString *qstr;
> > >+
> > >+    if (!data) {
> > >+        return NULL;
> > >+    }
> > >+    qstr = qobject_to_qstring(data);
> > >+    if (qstr) {
> > >+        return qstring_get_str(qstr);
> > >+    } else {
> > >+        return NULL;
> > >+    }
> > >+}
> > >+
> > >+static QObject *get_definition(const char *str, bool update_path)
> > >+{
> > >+    QObject *data, *value;
> > >+    QDict *qdict;
> > >+    int i;
> > >+
> > >+    if (!strcmp(str, "str") || !strcmp(str, "int") ||
> > >+        !strcmp(str, "number") || !strcmp(str, "bool") ||
> > >+        !strcmp(str, "int8") || !strcmp(str, "int16") ||
> > >+        !strcmp(str, "int32") || !strcmp(str, "int64") ||
> > >+        !strcmp(str, "uint8") || !strcmp(str, "uint16") ||
> > >+        !strcmp(str, "uint32") || !strcmp(str, "uint64") ||
> > >+        !strcmp(str, "visitor") || !strcmp(str, "**") ||
> > >+        !strcmp(str, "size")) {
> > >+        return NULL;
> > >+    }
> > >+
> > >+    for (i = 0; qmp_schema_table[i]; i++) {
> > >+        data = qobject_from_json(qmp_schema_table[i]);
> > >+        qdict = qobject_to_qdict(data);
> > >+        assert(qdict != NULL);
> > >+
> > >+        if (qdict_get(qdict, "enum")) {
> > >+            value = qdict_get(qdict, "enum");
> > >+        } else if (qdict_get(qdict, "type")) {
> > >+            value = qdict_get(qdict, "type");
> > >+        } else if (qdict_get(qdict, "union")) {
> > >+            value = qdict_get(qdict, "union");
> > >+        } else {
> > >+            continue;
> > >+        }
> > >+
> > >+        if (!strcmp(str, qstring_copy_str(value)) && update_path) {
> > 
> > Simply:
> > 
> > if (strcmp(str, qstring_copy_str(value)) {
> >     continue;
> > }
> > 
> > Then avoid another check for this.
> > 
> > >+            char *start, *end;
> > >+            char cur_idx[256];
> > >+            char type_idx[256];
> > >+
> > >+            start = visit_path_str;
> > >+            sprintf(type_idx, "%d", i);
> > 
> > Let's avoid sprintf completely.
> > 
> > >+            while (start) {
> > >+                end = strchr(start, ':');
> > >+                if (!end) {
> > >+                    break;
> > >+                }
> > >+                snprintf(cur_idx, end - start + 1, "%s", start);
> > >+                start = end + 1;
> > >+                /* if the type was already extended in parent node,
> > >+                 * we don't extend it again to avoid dead loop. */
> > >+                if (!strcmp(cur_idx, type_idx)) {
> > >+                    return NULL;
> > >+                }
> > >+            }
> > 
> > Using array of int will get you a simpler code here.
> > 
> > Furthermore, you could avoid linear lookup by using a bitmap beside
> > visit_path_str to keep track of what is visited.
> > 
> > >+            /* push index to visit_path_str before extending */
> > >+            push_id(i);
> > >+        }
> > >+        if (!strcmp(str, qstring_copy_str(value))) {
> > >+
> > >+            return qobject_from_json(qmp_schema_table[i]);
> > >+        }
> > >+    }
> > >+    return NULL;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_dict(QObject *data);
> > >+static DataObject *visit_qobj_list(QObject *data);
> > >+
> > >+/* extend defined type to json object */
> > >+static DataObject *extend_type(const char *str)
> > >+{
> > >+    QObject *data;
> > >+    DataObject *obj;
> > >+
> > >+    data = get_definition(str, true);
> > >+
> > >+    if (data) {
> > >+        obj = visit_qobj_dict(data);
> > >+        pop_id();
> > >+    } else {
> > >+        obj = g_malloc0(sizeof(struct DataObject));
> > >+        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> > >+        obj->reference_type = g_malloc0(sizeof(String));
> > >+        obj->reference_type->str = g_strdup(str);
> > >+    }
> > >+
> > >+    return obj;
> > >+}
> > >+
> > >+static DataObjectMemberList *list_to_memberlist(QObject *data)
> > >+{
> > >+    DataObjectMemberList *mem_list, *entry, *last_entry;
> > >+    QList *qlist;
> > >+    const QListEntry *lent;
> > >+
> > >+        qlist = qobject_to_qlist(data);
> > 
> > Bad indentation starting from here.
> > 
> > >+
> > >+        mem_list = NULL;
> > >+        for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >+            entry = g_malloc0(sizeof(DataObjectMemberList *));
> > 
> > Pointer size allocated instead of structure.
> > 
> > >+            entry->value = g_malloc0(sizeof(DataObjectMember));
> > >+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> > >+
> > >+            if (get_definition(qstring_copy_str(lent->value), false)) {
> > >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+                entry->value->has_recursive = true;
> > >+                entry->value->recursive = true;
> > >+                entry->value->type->extend =
> > >+                    extend_type(qstring_copy_str(lent->value));
> > >+            } else {
> > >+                entry->value->type->kind =
> > >+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > >+                entry->value->has_recursive = true;
> > >+                entry->value->recursive = false;
> > >+                entry->value->type->reference =
> > >+                    g_strdup(qstring_copy_str(lent->value));
> > >+            }
> > >+
> > >+            entry->next = NULL;
> > >+            if (!mem_list) {
> > >+                mem_list = entry;
> > >+            } else {
> > >+                last_entry->next = entry;
> > >+            }
> > >+            last_entry = entry;
> > >+        }
> > >+        return mem_list;
> > >+}
> > >+
> > >+static DataObjectMemberList *dict_to_memberlist(QObject *data)
> > >+{
> > >+    DataObjectMemberList *mem_list, *entry, *last_entry;
> > >+    QDict *qdict;
> > >+    const QDictEntry *dent;
> > >+
> > >+        qdict = qobject_to_qdict(data);
> > >+
> > >+        mem_list = NULL;
> > >+        for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> > >+            entry = g_malloc0(sizeof(DataObjectMemberList *));
> > 
> > Same here.
> > 
> > >+            entry->value = g_malloc0(sizeof(DataObjectMember));
> > >+
> > >+            entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> > >+
> > >+            if (dent->value->type->code == QTYPE_QDICT) {
> > >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+                entry->value->type->extend = visit_qobj_dict(dent->value);
> > >+            } else if (dent->value->type->code == QTYPE_QLIST) {
> > >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+                entry->value->type->extend = visit_qobj_list(dent->value);
> > >+            } else if (get_definition(qstring_copy_str(dent->value), false)) {
> > >+                entry->value->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+                entry->value->has_recursive = true;
> > >+                entry->value->recursive = true;
> > >+                entry->value->type->extend =
> > >+                    extend_type(qstring_copy_str(dent->value));
> > >+            } else {
> > >+                entry->value->type->kind =
> > >+                    DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > >+                entry->value->has_recursive = true;
> > >+                entry->value->recursive = false;
> > >+                entry->value->type->reference =
> > >+                    g_strdup(qstring_copy_str(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);
> > >+            }
> > >+
> > >+            entry->next = NULL;
> > >+            if (!mem_list) {
> > >+                mem_list = entry;
> > >+            } else {
> > >+                last_entry->next = entry;
> > >+            }
> > >+            last_entry = entry;
> > >+        }
> > >+        return mem_list;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_list(QObject *data)
> > >+{
> > >+    DataObject *obj;
> > >+
> > >+    obj = g_malloc0(sizeof(struct DataObject));
> > >+    obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> > >+    obj->anonymous_struct = g_malloc0(sizeof(struct
> > >+                                             DataObjectAnonymousStruct));
> > >+    obj->anonymous_struct->data = list_to_memberlist(data);
> > >+
> > >+    return obj;
> > >+}
> > >+
> > >+static strList *get_str_list(QObject *data)
> > >+{
> > >+    strList *str_list, *last_str_entry, *str_entry;
> > >+    QList *qlist;
> > >+    const QListEntry *lent;
> > >+
> > >+    qlist = qobject_to_qlist(data);
> > >+    str_list = NULL;
> > >+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >+        str_entry = g_malloc0(sizeof(strList *));
> > 
> > And here.
> > 
> > >+        str_entry->value = g_strdup(qstring_copy_str(lent->value));
> > >+        str_entry->next = NULL;
> > >+        if (!str_list) {
> > >+            str_list = str_entry;
> > >+        } else {
> > >+            last_str_entry->next = str_entry;
> > >+        }
> > >+        last_str_entry = str_entry;
> > >+    }
> > >+
> > >+    return str_list;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_dict(QObject *data)
> > >+{
> > >+    DataObject *obj;
> > >+    QObject *subdata;
> > >+    QDict *qdict;
> > >+
> > >+    qdict = qobject_to_qdict(data);
> > >+    assert(qdict != NULL);
> > >+    obj = g_malloc0(sizeof(*obj));
> > >+
> > >+    if (qdict_get(qdict, "command")) {
> > >+        obj->kind = DATA_OBJECT_KIND_COMMAND;
> > >+        obj->has_name = true;
> > >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "command")));
> > >+        obj->command = g_malloc0(sizeof(struct DataObjectCommand));
> > >+
> > >+        subdata = qdict_get(qdict, "data");
> > >+        if (subdata && subdata->type->code == QTYPE_QDICT) {
> > >+            obj->command->has_data = true;
> > >+            obj->command->data = dict_to_memberlist(subdata);
> > >+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> > >+            abort();
> > >+        } else if (subdata) {
> > >+            obj->command->has_data = true;
> > >+            obj->command->data =
> > >+                dict_to_memberlist(get_definition(qstring_copy_str(subdata),
> > >+                                                  true));
> > >+            pop_id();
> > >+        }
> > >+
> > >+        subdata = qdict_get(qdict, "returns");
> > >+        if (subdata && subdata->type->code == QTYPE_QDICT) {
> > >+            abort();
> > >+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> > >+            obj->command->has_returns = true;
> > >+            obj->command->returns = visit_qobj_list(subdata);
> > >+        } else if (subdata && subdata->type->code == QTYPE_QSTRING) {
> > >+            obj->command->has_returns = true;
> > >+            obj->command->returns = extend_type(qstring_copy_str(subdata));
> > >+        }
> > >+
> > >+        subdata = qdict_get(qdict, "gen");
> > >+        if (subdata && subdata->type->code == QTYPE_QSTRING) {
> > >+            obj->command->has_gen = true;
> > >+            if (!strcmp(qstring_copy_str(subdata), "no")) {
> > >+                obj->command->gen = false;
> > >+            } else {
> > >+                obj->command->gen = true;
> > >+            }
> > >+        }
> > >+    } else if (qdict_get(qdict, "union")) {
> > >+        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> > >+        obj->has_name = true;
> > >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "union")));
> > >+        obj->unionobj = g_malloc0(sizeof(struct DataObjectUnion));
> > >+        subdata = qdict_get(qdict, "data");
> > >+        obj->unionobj->data = dict_to_memberlist(subdata);
> > >+    } else if (qdict_get(qdict, "type")) {
> > >+        obj->kind = DATA_OBJECT_KIND_TYPE;
> > >+        obj->has_name = true;
> > >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "type")));
> > >+        obj->type = g_malloc0(sizeof(struct DataObjectType));
> > >+        subdata = qdict_get(qdict, "data");
> > >+        obj->type->data = dict_to_memberlist(subdata);
> > >+    } else if (qdict_get(qdict, "enum")) {
> > >+        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> > >+        obj->has_name = true;
> > >+        obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "enum")));
> > >+        obj->enumeration = g_malloc0(sizeof(struct DataObjectEnumeration));
> > >+        subdata = qdict_get(qdict, "data");
> > >+        obj->enumeration->data = get_str_list(subdata);
> > >+    } else {
> > >+        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> > >+        obj->anonymous_struct = g_malloc0(sizeof(struct
> > >+                                                 DataObjectAnonymousStruct));
> > >+        obj->anonymous_struct->data = dict_to_memberlist(data);
> > >+    }
> > >+
> > >+    return obj;
> > >+}
> > >+
> > >+DataObjectList *qmp_query_qmp_schema(Error **errp)
> > >+{
> > >+    DataObjectList *list, *last_entry, *entry;
> > >+    QObject *data;
> > >+    int i;
> > >+
> > >+    list = NULL;
> > >+    for (i = 0; qmp_schema_table[i]; i++) {
> > >+        data = qobject_from_json(qmp_schema_table[i]);
> > >+        assert(data != NULL);
> > >+
> > >+        entry = g_malloc0(sizeof(DataObjectList *));
> > 
> > And here.
> > 
> > >+        memset(visit_path_str, 0, sizeof(visit_path_str));
> > >+        entry->value = visit_qobj_dict(data);
> > >+        entry->next = NULL;
> > >+        if (!list) {
> > 
> > Why not use a pointer to pointer to avoid this if branch?
>  
> Will fix it.
> 
> > >+            list = entry;
> > >+        } else {
> > >+            last_entry->next = entry;
> > >+        }
> > >+        last_entry = entry;
> > >+    }
> > >+
> > >+    return list;
> > >+}
> > >+
> > >  void qmp_add_client(const char *protocol, const char *fdname,
> > >                      bool has_skipauth, bool skipauth, bool has_tls, bool tls,
> > >                      Error **errp)
> > >
> > 
> > Fam
>