* [Qemu-devel] [PATCH v2 0/2] QMP full introspection
@ 2013-07-16 10:37 Amos Kong
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json Amos Kong
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
0 siblings, 2 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-16 10:37 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, armbru, lcapitulino
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.
Anthony has another suggestion:
http://lists.nongnu.org/archive/html/qemu-devel/2013-07/msg01903.html
The whole output of query-qmp-schema command:
https://raw.github.com/kongove/misc/master/txt/query-qmp-schema.txt
Welcome your comments!
Amos Kong (2):
qapi: change qapi to convert schema json
full introspection support for QMP
Makefile | 5 +-
docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
qapi-schema.json | 69 +++++++++
qmp-commands.hx | 39 +++++
qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
scripts/qapi-commands.py | 2 +-
scripts/qapi-types.py | 47 +++++-
scripts/qapi-visit.py | 2 +-
scripts/qapi.py | 4 +-
9 files changed, 611 insertions(+), 7 deletions(-)
create mode 100644 docs/qmp-full-introspection.txt
--
1.8.3.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json
2013-07-16 10:37 [Qemu-devel] [PATCH v2 0/2] QMP full introspection Amos Kong
@ 2013-07-16 10:37 ` Amos Kong
2013-07-17 20:09 ` Luiz Capitulino
2013-07-19 12:27 ` Eric Blake
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
1 sibling, 2 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-16 10:37 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, armbru, lcapitulino
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.
eg:
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++---
scripts/qapi-visit.py | 2 +-
scripts/qapi.py | 4 +++-
5 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/Makefile b/Makefile
index c06bfab..2348bce 100644
--- a/Makefile
+++ b/Makefile
@@ -38,7 +38,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
@@ -223,6 +223,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 "." -i "$@" < $<, " 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 e06332b..d15d04f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -437,7 +437,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 ddcfed9..0327825 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:
@@ -204,9 +205,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:], "chbi:p:o:",
["source", "header", "builtins",
- "prefix=", "output-dir="])
+ "introspect-file=", "prefix=",
+ "output-dir="])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -215,6 +217,7 @@ output_dir = ""
prefix = ""
c_file = 'qapi-types.c'
h_file = 'qapi-types.h'
+introspect_file = ""
do_c = False
do_h = False
@@ -231,11 +234,17 @@ for o, a in opts:
do_h = True
elif o in ("-b", "--builtins"):
do_builtins = True
+ elif o in ("-i", "--instrospect-file"):
+ introspect_file = a
if not do_c and not do_h:
do_c = True
do_h = True
+if introspect_file:
+ do_c = False
+ do_h = False
+
c_file = output_dir + prefix + c_file
h_file = output_dir + prefix + h_file
@@ -303,7 +312,39 @@ 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 introspect_file:
+ for line in exprs_all[1]:
+ line = re.sub(r'\n', ' ', line.strip())
+ line = re.sub(r' +', ' ', line)
+ schema_table += ' "%s",\n' % (line)
+
+ schema_table += ' NULL };\n'
+ f = open(introspect_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 6cac05a..70f80eb 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -334,7 +334,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 baf1321..5674003 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -98,9 +98,11 @@ def get_expr(fp):
def parse_schema(fp):
exprs = []
+ raw_exprs = []
for expr in get_expr(fp):
expr_eval = evaluate(expr)
+ raw_exprs.append(expr)
if expr_eval.has_key('enum'):
add_enum(expr_eval['enum'])
@@ -110,7 +112,7 @@ def parse_schema(fp):
add_struct(expr_eval)
exprs.append(expr_eval)
- return exprs
+ return exprs, raw_exprs
def parse_args(typeinfo):
if isinstance(typeinfo, basestring):
--
1.8.3.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 10:37 [Qemu-devel] [PATCH v2 0/2] QMP full introspection Amos Kong
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json Amos Kong
@ 2013-07-16 10:37 ` Amos Kong
2013-07-16 10:48 ` Paolo Bonzini
` (2 more replies)
1 sibling, 3 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-16 10:37 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, armbru, lcapitulino
Introduces new monitor command to query QMP schema information,
the return data is a dynamical and nested dict/list, it contains
the useful metadata to help management to check feature support,
QMP commands detail, etc.
I added a document for QMP introspection support.
(docs/qmp-full-introspection.txt)
We need to parse all commands json definition, and generated a
dynamical tree, QMP infrastructure will convert the tree to
json string and return to QMP client.
So here I defined a 'DataObject' type in qapi-schema.json,
it's used to describe the dynamical dictionary/list/string.
{ 'type': 'DataObject',
'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
Not all the keys in data will be used.
# List: type
# Dict: key, type
# nested List: type, data
# nested Dict: key, type, data
The DataObject is described in docs/qmp-full-introspection.txt in
detail.
The following content gives an example of query-tpm-types:
## Define example in qapi-schema.json:
{ 'enum': 'TpmType', 'data': [ 'passthrough' ] }
{ 'command': 'query-tpm-types', 'returns': ['TpmType'] }
## Returned description:
{
"name": "query-tpm-types",
"type": "Command",
"returns": [
{
"type": "TpmType",
"data": [
{
"type": "passthrough"
}
]
}
]
},
'TpmType' is a defined type, it will be extended in returned
description. [ 'passthrough' ] is a list, so 'type' and 'data'
will be used.
TODO:
We can add events definations to qapi-schema.json by another
patch, then event can also be queried.
Introduce another command 'query-qga-schema' to query QGA schema
information, it's easy to add this support with current current
patch.
Signed-off-by: Amos Kong <akong@redhat.com>
---
docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
qapi-schema.json | 69 +++++++++
qmp-commands.hx | 39 +++++
qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 558 insertions(+)
create mode 100644 docs/qmp-full-introspection.txt
diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
new file mode 100644
index 0000000..cc0fb80
--- /dev/null
+++ b/docs/qmp-full-introspection.txt
@@ -0,0 +1,143 @@
+= full introspection support for QMP =
+
+== Purpose ==
+
+Add a new interface to provide QMP schema information to management,
+the return data is a dynamical and nested dict/list, it contains
+the useful metadata to help management to check feature support,
+QMP commands detail, etc.
+
+== Usage ==
+
+Execute QMP command:
+
+ { "execute": "query-qmp-schema" }
+
+Returns:
+
+ { "return": [
+ {
+ "name": "query-name",
+ "type": "Command",
+ "returns": [
+ {
+ "key": "*name",
+ "type": "str"
+ }
+ ]
+ },
+ ...
+ }
+
+The whole schema information will be returned in one go, it contains
+commands and event. It doesn't support to be filtered by type or name.
+
+We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
+that uses themself in their own define data directly or indirectly,
+we will not repeatedly extend them to avoid dead loop.
+
+== more description of 'DataObject' type ==
+
+We use 'DataObject' to describe dynamical data struct, it might be
+nested dictionary, list or string.
+
+'DataObject' itself is a arbitrary and nested dictionary, the
+dictionary has three keys ('key', 'type', 'data'), 'key' and
+'data' are optional.
+
+* For describing Dictionary, we set the key to 'key', and set the
+ value to 'type'
+* For describing List, we don't set 'key', just set the value to
+ 'type'
+* If the value of dictionary or list is non-native type, we extend
+ the non-native type to dictionary, set it to 'data', and set the
+ non-native type's name to 'type'.
+* If the value of dictionary or list is dictionary or list, 'type'
+ won't be set.
+
+== examples ==
+
+1) Dict, value is native type
+{ 'id': 'str', ... }
+--------------------
+[
+ {
+ "key": "id",
+ "type": "str"
+ },
+ .....
+]
+
+2) Dict, value is defined types
+{ 'options': 'TpmTypeOptions' }
+-------------------------------
+[
+ {
+ "key": "options",
+ "type": "TpmTypeOptions",
+ "data": [
+ {
+ "key": "passthrough",
+ "type": "str",
+ }
+ ]
+ },
+ .....
+]
+
+3) List, value is native type
+['str', ... ]
+-------------
+[
+ {
+ "type": "str"
+ },
+ ....
+]
+
+4) List, value is defined types
+['TpmTypeOptions', ... ]
+------------------------
+[
+ {
+ "type": "TpmTypeOptions",
+ "data": [
+ {
+ "key": "passthrough",
+ "type": "str",
+ }
+ ]
+ },
+ .....
+]
+
+5) Dict, value is dictionary
+{ 'info': { 'age': 'init', ... } }
+-----------------------------
+[
+ {
+ "key": "info",
+ "data": [
+ {
+ "key": "age",
+ "type": "init",
+ },
+ ...
+ ]
+ },
+]
+
+6) Dict, value is list
+{ 'info': [ 'str', ... } }
+-----------------------------
+[
+ {
+ "key": "info",
+ "data": [
+ {
+ "type": "str",
+ },
+ ...
+ ]
+ },
+]
diff --git a/qapi-schema.json b/qapi-schema.json
index 7b9fef1..cf03391 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3679,3 +3679,72 @@
'*cpuid-input-ecx': 'int',
'cpuid-register': 'X86CPURegister32',
'features': 'int' } }
+
+##
+# @DataObject
+#
+# Details of a data object, it can be nested dictionary/list
+#
+# @key: #optional Data object key
+#
+# @type: Data object type name
+#
+# @optional: #optional whether the data object is optional
+#
+# @data: #optional DataObject list, can be a dictionary or list type
+#
+# Since: 1.6
+##
+{ 'type': 'DataObject',
+ 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
+
+##
+# @SchemaMetaType
+#
+# Possible meta types of a schema entry
+#
+# @Command: QMP command
+#
+# @Type: defined new data type
+#
+# @Enumeration: enumeration data type
+#
+# @Union: union data type
+#
+# Since: 1.6
+##
+{ 'enum': 'SchemaMetaType',
+ 'data': ['Command', 'Type', 'Enumeration', 'Union'] }
+
+##
+# @SchemaEntry
+#
+# Details of schema items
+#
+# @type: Entry's type in string format
+#
+# @name: Entry name
+#
+# @data: #optional list of DataObject. This can have different meaning
+# depending on the 'type' value. For example, for a QMP command,
+# this member contains an argument listing. For an enumeration,
+# it contains the enum's values and so on
+#
+# @returns: #optional list of DataObject, return data after executing
+# QMP command
+#
+# Since: 1.6
+##
+{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
+ 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
+
+##
+# @query-qmp-schema
+#
+# Query QMP schema information
+#
+# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
+#
+# Since: 1.6
+##
+{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e075df4..e3cbe93 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3047,3 +3047,42 @@ Example:
<- { "return": {} }
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', 'event'
+- "data": schema data (json-object, optional)
+- "returns": return data of qmp command (json-object, optional)
+
+Example:
+
+-> { "execute": "query-qmp-schema" }
+<- { "return": [
+ {
+ "name": "query-name",
+ "type": "Command",
+ "returns": [
+ {
+ "key": "*name",
+ "type": "str"
+ }
+ ]
+ }
+ ]
+ }
+
+EQMP
diff --git a/qmp.c b/qmp.c
index 4c149b3..3ace3a6 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,311 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
return arch_query_cpu_definitions(errp);
}
+/*
+ * Use a string to record the visit path, type index of each node
+ * will be saved to the string, indexes are split by ':'.
+ */
+static char visit_path_str[1024];
+
+/* push the type index to 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 DataObjectList *visit_qobj_dict(QObject *data);
+static DataObjectList *visit_qobj_list(QObject *data);
+
+/* extend defined type to json object */
+static DataObjectList *extend_type(const char* str)
+{
+ DataObjectList *data_list;
+ QObject *data;
+ QDict *qdict;
+ const QDictEntry *ent;
+ int i;
+
+ /* don't extend builtin types */
+ 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")) {
+ return NULL;
+ }
+
+ for (i = 0; qmp_schema_table[i]; i++) {
+ data = qobject_from_json(qmp_schema_table[i]);
+ assert(data != NULL);
+
+ qdict = qobject_to_qdict(data);
+ assert(qdict != NULL);
+
+ ent = qdict_first(qdict);
+ if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
+ && !qdict_get(qdict, "union")) {
+ continue;
+ }
+
+ if (!strcmp(str, qstring_copy_str(ent->value))) {
+ 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);
+
+ data = qdict_get(qdict, "data");
+ if(data) {
+ if (data->type->code == QTYPE_QDICT) {
+ data_list = visit_qobj_dict(data);
+ } else if (data->type->code == QTYPE_QLIST) {
+ data_list = visit_qobj_list(data);
+ }
+ /* pop index from visit_path_str after extending */
+ pop_id();
+
+ return data_list;
+ }
+ }
+ }
+
+ return NULL;
+}
+
+static DataObjectList *visit_qobj_list(QObject *data)
+{
+ DataObjectList *obj_list, *obj_last_entry, *obj_entry;
+ DataObject *obj_info;
+ const QListEntry *ent;
+ QList *qlist;
+
+ qlist = qobject_to_qlist(data);
+ assert(qlist != NULL);
+
+ obj_list = NULL;
+ for (ent = qlist_first(qlist); ent; ent = qlist_next(ent)) {
+ obj_info = g_malloc0(sizeof(*obj_info));
+ obj_entry = g_malloc0(sizeof(*obj_entry));
+ obj_entry->value = obj_info;
+ obj_entry->next = NULL;
+
+ if (!obj_list) {
+ obj_list = obj_entry;
+ } else {
+ obj_last_entry->next = obj_entry;
+ }
+ obj_last_entry = obj_entry;
+
+ if (ent->value->type->code == QTYPE_QDICT) {
+ obj_info->data = visit_qobj_dict(ent->value);
+ } else if (ent->value->type->code == QTYPE_QLIST) {
+ obj_info->data = visit_qobj_list(ent->value);
+ } else if (ent->value->type->code == QTYPE_QSTRING) {
+ obj_info->has_type = true;
+ obj_info->type = g_strdup(qstring_copy_str(ent->value));
+ obj_info->data = extend_type(qstring_copy_str(ent->value));
+ }
+ if (obj_info->data) {
+ obj_info->has_data = true;
+ }
+ }
+
+ return obj_list;
+}
+
+static DataObjectList *visit_qobj_dict(QObject *data)
+{
+ DataObjectList *obj_list, *obj_last_entry, *obj_entry;
+ DataObject *obj_info;
+ const QDictEntry *ent;
+ QDict *qdict;
+
+ qdict = qobject_to_qdict(data);
+ assert(qdict != NULL);
+
+ obj_list = NULL;
+ for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
+ obj_info = g_malloc0(sizeof(*obj_info));
+ obj_entry = g_malloc0(sizeof(*obj_entry));
+ obj_entry->value = obj_info;
+ obj_entry->next = NULL;
+
+ if (!obj_list) {
+ obj_list = obj_entry;
+ } else {
+ obj_last_entry->next = obj_entry;
+ }
+ obj_last_entry = obj_entry;
+
+ if (ent->key[0] == '*') {
+ obj_info->key = g_strdup(ent->key + 1);
+ obj_info->has_optional = true;
+ obj_info->optional = true;
+ } else {
+ obj_info->key = g_strdup(ent->key);
+ }
+ obj_info->has_key = true;
+
+ if (ent->value->type->code == QTYPE_QDICT) {
+ obj_info->data = visit_qobj_dict(ent->value);
+ } else if (ent->value->type->code == QTYPE_QLIST) {
+ obj_info->data = visit_qobj_list(ent->value);
+ } else if (ent->value->type->code == QTYPE_QSTRING) {
+ obj_info->has_type = true;
+ obj_info->type = g_strdup(qstring_copy_str(ent->value));
+ obj_info->data = extend_type(qstring_copy_str(ent->value));
+ }
+ if (obj_info->data) {
+ obj_info->has_data = true;
+ }
+ }
+
+ return obj_list;
+}
+
+SchemaEntryList *qmp_query_qmp_schema(Error **errp)
+{
+ SchemaEntryList *list, *last_entry, *entry;
+ SchemaEntry *info;
+ DataObjectList *obj_entry;
+ DataObject *obj_info;
+ QObject *data;
+ QDict *qdict;
+ int i;
+
+ list = NULL;
+ for (i = 0; qmp_schema_table[i]; i++) {
+ data = qobject_from_json(qmp_schema_table[i]);
+ assert(data != NULL);
+
+ qdict = qobject_to_qdict(data);
+ assert(qdict != NULL);
+
+ if (qdict_get(qdict, "command")) {
+ info = g_malloc0(sizeof(*info));
+ info->type = SCHEMA_META_TYPE_COMMAND;
+ info->name = strdup(qdict_get_str(qdict, "command"));
+ } else {
+ continue;
+ }
+
+ memset(visit_path_str, 0, sizeof(visit_path_str));
+ data = qdict_get(qdict, "data");
+ if (data) {
+ info->has_data = true;
+ if (data->type->code == QTYPE_QLIST) {
+ info->data = visit_qobj_list(data);
+ } else if (data->type->code == QTYPE_QDICT) {
+ info->data = visit_qobj_dict(data);
+ } else if (data->type->code == QTYPE_QSTRING) {
+ info->data = extend_type(qstring_get_str(qobject_to_qstring(data)));
+ if (!info->data) {
+ obj_info = g_malloc0(sizeof(*obj_info));
+ obj_entry = g_malloc0(sizeof(*obj_entry));
+ obj_entry->value = obj_info;
+ obj_info->has_type = true;
+ obj_info->type = g_strdup(qdict_get_str(qdict, "data"));
+ info->data = obj_entry;
+ }
+ } else {
+ abort();
+ }
+ }
+
+ memset(visit_path_str, 0, sizeof(visit_path_str));
+ data = qdict_get(qdict, "returns");
+ if (data) {
+ info->has_returns = true;
+ if (data->type->code == QTYPE_QLIST) {
+ info->returns = visit_qobj_list(data);
+ } else if (data->type->code == QTYPE_QDICT) {
+ info->returns = visit_qobj_dict(data);
+ } else if (data->type->code == QTYPE_QSTRING) {
+ info->returns = extend_type(qstring_copy_str(data));
+ if (!info->returns) {
+ obj_info = g_malloc0(sizeof(*obj_info));
+ obj_entry = g_malloc0(sizeof(*obj_entry));
+ obj_entry->value = obj_info;
+ obj_info->has_type = true;
+ obj_info->type = g_strdup(qdict_get_str(qdict, "returns"));
+ info->returns = obj_entry;
+ }
+ }
+ }
+
+ entry = g_malloc0(sizeof(DataObjectList *));
+ entry->value = info;
+ 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.3.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
@ 2013-07-16 10:48 ` Paolo Bonzini
2013-07-16 11:04 ` Amos Kong
2013-07-17 20:36 ` Luiz Capitulino
2013-07-19 22:05 ` Eric Blake
2 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2013-07-16 10:48 UTC (permalink / raw)
To: Amos Kong; +Cc: armbru, aliguori, qemu-devel, lcapitulino
Il 16/07/2013 12:37, Amos Kong ha scritto:
> So here I defined a 'DataObject' type in qapi-schema.json,
> it's used to describe the dynamical dictionary/list/string.
>
> { 'type': 'DataObject',
> 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
This is missing '*optional': 'bool'. Also, how do you distinguish these:
{ 'command': 'query-tpm-types', 'returns': 'TpmType] }
{ 'command': 'query-tpm-types', 'returns': ['TpmType'] }
Could it have to be like this?
'data': { '*key': 'str', '*type': 'str', '*list': 'bool',
'*optional': 'bool',
'*data': ['DataObject'] } }
Can you document, in the commit message or the code, how you avoid
infinite loops (possible with optional or list fields)?
Paolo
> Not all the keys in data will be used.
> # List: type
> # Dict: key, type
> # nested List: type, data
> # nested Dict: key, type, data
>
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
>
> The following content gives an example of query-tpm-types:
>
> ## Define example in qapi-schema.json:
>
> { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>
> ## Returned description:
>
> {
> "name": "query-tpm-types",
> "type": "Command",
> "returns": [
> {
> "type": "TpmType",
> "data": [
> {
> "type": "passthrough"
> }
> ]
> }
> ]
> },
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 10:48 ` Paolo Bonzini
@ 2013-07-16 11:04 ` Amos Kong
2013-07-16 11:08 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Amos Kong @ 2013-07-16 11:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: armbru, aliguori, qemu-devel, lcapitulino
On Tue, Jul 16, 2013 at 12:48:36PM +0200, Paolo Bonzini wrote:
> Il 16/07/2013 12:37, Amos Kong ha scritto:
> > So here I defined a 'DataObject' type in qapi-schema.json,
> > it's used to describe the dynamical dictionary/list/string.
> >
> > { 'type': 'DataObject',
> > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
Hi Paolo,
> This is missing '*optional': 'bool'. Also, how do you distinguish these:
>
> { 'command': 'query-tpm-types', 'returns': 'TpmType] }
do you mean 'TpmType' ? not 'TpmType]
{
"name": "query-tpm-types",
"type": "Command",
"returns": [
> {
> "type": "passthrough"
> }
]
},
> { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
{
"name": "query-tpm-types",
"type": "Command",
"returns": [
> {
> "type": "TpmType",
> "data": [
> {
> "type": "passthrough"
> }
> ]
> }
]
},
>
> Could it have to be like this?
>
> 'data': { '*key': 'str', '*type': 'str', '*list': 'bool',
> '*optional': 'bool',
> '*data': ['DataObject'] } }
there are three conditions:
1) list
2) dict
3) string
> Can you document, in the commit message or the code,
I added a document for QMP introspection support.
(docs/qmp-full-introspection.txt)
The DataObject is described in docs/qmp-full-introspection.txt in
detail.
> how you avoid infinite loops (possible with optional or list fields)?
+We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
+that uses themself in their own define data directly or indirectly,
+we will not repeatedly extend them to avoid dead loop.
+/*
+ * Use a string to record the visit path, type index of each node
+ * will be saved to the string, indexes are split by ':'.
+ */
related functions:
pop_id()
push_id()
The detail needs to be added to qmp-full-introspection.txt
> Paolo
>
> > Not all the keys in data will be used.
> > # List: type
> > # Dict: key, type
> > # nested List: type, data
> > # nested Dict: key, type, data
> >
> > The DataObject is described in docs/qmp-full-introspection.txt in
> > detail.
> >
> > The following content gives an example of query-tpm-types:
> >
> > ## Define example in qapi-schema.json:
> >
> > { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> > { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
> >
> > ## Returned description:
> >
> > {
> > "name": "query-tpm-types",
> > "type": "Command",
> > "returns": [
> > {
> > "type": "TpmType",
> > "data": [
> > {
> > "type": "passthrough"
> > }
> > ]
> > }
> > ]
> > },
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 11:04 ` Amos Kong
@ 2013-07-16 11:08 ` Paolo Bonzini
2013-07-16 12:04 ` Amos Kong
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2013-07-16 11:08 UTC (permalink / raw)
To: Amos Kong; +Cc: armbru, aliguori, qemu-devel, lcapitulino
Il 16/07/2013 13:04, Amos Kong ha scritto:
>>> > > So here I defined a 'DataObject' type in qapi-schema.json,
>>> > > it's used to describe the dynamical dictionary/list/string.
>>> > >
>>> > > { 'type': 'DataObject',
>>> > > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
>
> Hi Paolo,
>
>> > This is missing '*optional': 'bool'. Also, how do you distinguish these:
>> >
>> > { 'command': 'query-tpm-types', 'returns': 'TpmType] }
> do you mean 'TpmType' ? not 'TpmType]
Yes.
> {
> "name": "query-tpm-types",
> "type": "Command",
> "returns": [
> > {
> > "type": "passthrough"
> > }
> ]
> },
>
>> > { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
> {
> "name": "query-tpm-types",
> "type": "Command",
> "returns": [
> > {
> > "type": "TpmType",
> > "data": [
> > {
> > "type": "passthrough"
> > }
> > ]
> > }
> ]
> },
Thanks. I see this is unique, but it is also not too intuitive.
So, could you add a "kind" field to DataObject that is an enum
(list/dict/scalar, or something like that)? This would make it easier
to parse (for humans at least, but I guess also for programs).
Paolo
>> >
>> > Could it have to be like this?
>> >
>> > 'data': { '*key': 'str', '*type': 'str', '*list': 'bool',
>> > '*optional': 'bool',
>> > '*data': ['DataObject'] } }
> there are three conditions:
> 1) list
> 2) dict
> 3) string
>
>> > Can you document, in the commit message or the code,
>
> I added a document for QMP introspection support.
> (docs/qmp-full-introspection.txt)
>
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
>
>> > how you avoid infinite loops (possible with optional or list fields)?
>
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
> +that uses themself in their own define data directly or indirectly,
> +we will not repeatedly extend them to avoid dead loop.
>
> +/*
> + * Use a string to record the visit path, type index of each node
> + * will be saved to the string, indexes are split by ':'.
> + */
>
> related functions:
> pop_id()
> push_id()
>
> The detail needs to be added to qmp-full-introspection.txt
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 11:08 ` Paolo Bonzini
@ 2013-07-16 12:04 ` Amos Kong
2013-07-16 12:18 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Amos Kong @ 2013-07-16 12:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: armbru, aliguori, qemu-devel, lcapitulino
On Tue, Jul 16, 2013 at 01:08:55PM +0200, Paolo Bonzini wrote:
> Il 16/07/2013 13:04, Amos Kong ha scritto:
> >>> > > So here I defined a 'DataObject' type in qapi-schema.json,
> >>> > > it's used to describe the dynamical dictionary/list/string.
> >>> > >
> >>> > > { 'type': 'DataObject',
> >>> > > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> >
> > Hi Paolo,
> >
> >> > This is missing '*optional': 'bool'. Also, how do you distinguish these:
> >> >
> >> > { 'command': 'query-tpm-types', 'returns': 'TpmType] }
> > do you mean 'TpmType' ? not 'TpmType]
>
> Yes.
>
> > {
> > "name": "query-tpm-types",
> > "type": "Command",
> > "returns": [
> > > {
> > > "type": "passthrough"
> > > }
> > ]
> > },
> >
> >> > { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
> > {
> > "name": "query-tpm-types",
> > "type": "Command",
> > "returns": [
> > > {
> > > "type": "TpmType",
> > > "data": [
> > > {
> > > "type": "passthrough"
> > > }
> > > ]
> > > }
> > ]
> > },
>
> Thanks. I see this is unique, but it is also not too intuitive.
>
> So, could you add a "kind" field to DataObject that is an enum
> (list/dict/scalar, or something like that)? This would make it easier
> to parse (for humans at least, but I guess also for programs).
I thought we can identify the kind by some judgment.
if the dict has key 'key', it's a dict
if no 'key', have 'type', it's a list
if only have 'type', it's a buildin type (or extended type that
doesn't need to be extended)
if no 'key', have 'type' & 'data', it's extended list type
if have 'key', 'type', 'data', it's extended dict type
I will added a 'kind' field to make it clearer.
KIND enum:
list
dict
str
scalar(bool): Or just simplely check if have 'data' key?
true/false
Amos
> Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 12:04 ` Amos Kong
@ 2013-07-16 12:18 ` Paolo Bonzini
2013-07-26 7:03 ` Amos Kong
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2013-07-16 12:18 UTC (permalink / raw)
To: Amos Kong; +Cc: armbru, aliguori, qemu-devel, lcapitulino
Il 16/07/2013 14:04, Amos Kong ha scritto:
>> > Thanks. I see this is unique, but it is also not too intuitive.
>> >
>> > So, could you add a "kind" field to DataObject that is an enum
>> > (list/dict/scalar, or something like that)? This would make it easier
>> > to parse (for humans at least, but I guess also for programs).
> I thought we can identify the kind by some judgment.
Yes, I understood that. Strictly speaking the kind is redundant, but it
seems to me that it makes the API easier to understand and use.
> if the dict has key 'key', it's a dict
> if no 'key', have 'type', it's a list
> if only have 'type', it's a buildin type (or extended type that
> doesn't need to be extended)
> if no 'key', have 'type' & 'data', it's extended list type
> if have 'key', 'type', 'data', it's extended dict type
>
> I will added a 'kind' field to make it clearer.
>
> KIND enum:
> list
> dict
> str
Why "str" and not "scalar" for a builtin type? It's not necessarily a
string, is it?
Paolo
> scalar(bool): Or just simplely check if have 'data' key?
> true/false
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json Amos Kong
@ 2013-07-17 20:09 ` Luiz Capitulino
2013-07-26 3:39 ` Amos Kong
2013-07-19 12:27 ` Eric Blake
1 sibling, 1 reply; 26+ messages in thread
From: Luiz Capitulino @ 2013-07-17 20:09 UTC (permalink / raw)
To: Amos Kong; +Cc: pbonzini, aliguori, qemu-devel, armbru
On Tue, 16 Jul 2013 18:37:41 +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.
>
> eg:
> 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> scripts/qapi-visit.py | 2 +-
> scripts/qapi.py | 4 +++-
> 5 files changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c06bfab..2348bce 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -38,7 +38,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
> @@ -223,6 +223,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 "." -i "$@" < $<, " GEN $@")
What about qemu-ga?
>
> 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 e06332b..d15d04f 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -437,7 +437,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 ddcfed9..0327825 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:
> @@ -204,9 +205,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:], "chbi:p:o:",
> ["source", "header", "builtins",
> - "prefix=", "output-dir="])
> + "introspect-file=", "prefix=",
I'd call it schema-dump-file or something like it.
> + "output-dir="])
> except getopt.GetoptError, err:
> print str(err)
> sys.exit(1)
> @@ -215,6 +217,7 @@ output_dir = ""
> prefix = ""
> c_file = 'qapi-types.c'
> h_file = 'qapi-types.h'
> +introspect_file = ""
>
> do_c = False
> do_h = False
> @@ -231,11 +234,17 @@ for o, a in opts:
> do_h = True
> elif o in ("-b", "--builtins"):
> do_builtins = True
> + elif o in ("-i", "--instrospect-file"):
> + introspect_file = a
>
> if not do_c and not do_h:
> do_c = True
> do_h = True
>
> +if introspect_file:
> + do_c = False
> + do_h = False
> +
> c_file = output_dir + prefix + c_file
> h_file = output_dir + prefix + h_file
>
> @@ -303,7 +312,39 @@ 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 introspect_file:
> + for line in exprs_all[1]:
> + line = re.sub(r'\n', ' ', line.strip())
> + line = re.sub(r' +', ' ', line)
> + schema_table += ' "%s",\n' % (line)
> +
> + schema_table += ' NULL };\n'
> + f = open(introspect_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 6cac05a..70f80eb 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -334,7 +334,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 baf1321..5674003 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -98,9 +98,11 @@ def get_expr(fp):
>
> def parse_schema(fp):
> exprs = []
> + raw_exprs = []
>
> for expr in get_expr(fp):
> expr_eval = evaluate(expr)
> + raw_exprs.append(expr)
>
> if expr_eval.has_key('enum'):
> add_enum(expr_eval['enum'])
> @@ -110,7 +112,7 @@ def parse_schema(fp):
> add_struct(expr_eval)
> exprs.append(expr_eval)
>
> - return exprs
> + return exprs, raw_exprs
>
> def parse_args(typeinfo):
> if isinstance(typeinfo, basestring):
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
2013-07-16 10:48 ` Paolo Bonzini
@ 2013-07-17 20:36 ` Luiz Capitulino
2013-07-19 20:26 ` Eric Blake
2013-07-26 7:21 ` Amos Kong
2013-07-19 22:05 ` Eric Blake
2 siblings, 2 replies; 26+ messages in thread
From: Luiz Capitulino @ 2013-07-17 20:36 UTC (permalink / raw)
To: Amos Kong; +Cc: pbonzini, aliguori, qemu-devel, armbru
On Tue, 16 Jul 2013 18:37:42 +0800
Amos Kong <akong@redhat.com> wrote:
> Introduces new monitor command to query QMP schema information,
> the return data is a dynamical and nested dict/list, it contains
> the useful metadata to help management to check feature support,
> QMP commands detail, etc.
>
> I added a document for QMP introspection support.
> (docs/qmp-full-introspection.txt)
>
> We need to parse all commands json definition, and generated a
> dynamical tree, QMP infrastructure will convert the tree to
> json string and return to QMP client.
>
> So here I defined a 'DataObject' type in qapi-schema.json,
> it's used to describe the dynamical dictionary/list/string.
I skimmed over the patch and made some comments, but my impression
is that this is getting too complex... Why did we move from letting
mngt query type by type (last version) to this version which returns
all commands and their input types? Does this satisfy libvirt needs?
>
> { 'type': 'DataObject',
> 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
>
> Not all the keys in data will be used.
> # List: type
> # Dict: key, type
> # nested List: type, data
> # nested Dict: key, type, data
>
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
>
> The following content gives an example of query-tpm-types:
>
> ## Define example in qapi-schema.json:
>
> { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>
> ## Returned description:
>
> {
> "name": "query-tpm-types",
> "type": "Command",
> "returns": [
> {
> "type": "TpmType",
> "data": [
> {
> "type": "passthrough"
> }
> ]
> }
> ]
> },
>
> 'TpmType' is a defined type, it will be extended in returned
> description. [ 'passthrough' ] is a list, so 'type' and 'data'
> will be used.
>
> TODO:
> We can add events definations to qapi-schema.json by another
> patch, then event can also be queried.
>
> Introduce another command 'query-qga-schema' to query QGA schema
> information, it's easy to add this support with current current
> patch.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
> qapi-schema.json | 69 +++++++++
> qmp-commands.hx | 39 +++++
> qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 558 insertions(+)
> create mode 100644 docs/qmp-full-introspection.txt
>
> diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> new file mode 100644
> index 0000000..cc0fb80
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,143 @@
> += full introspection support for QMP =
> +
> +== Purpose ==
> +
> +Add a new interface to provide QMP schema information to management,
> +the return data is a dynamical and nested dict/list, it contains
> +the useful metadata to help management to check feature support,
> +QMP commands detail, etc.
> +
> +== Usage ==
> +
> +Execute QMP command:
> +
> + { "execute": "query-qmp-schema" }
> +
> +Returns:
> +
> + { "return": [
> + {
> + "name": "query-name",
> + "type": "Command",
> + "returns": [
> + {
> + "key": "*name",
> + "type": "str"
> + }
> + ]
> + },
> + ...
> + }
> +
> +The whole schema information will be returned in one go, it contains
> +commands and event. It doesn't support to be filtered by type or name.
> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
> +that uses themself in their own define data directly or indirectly,
> +we will not repeatedly extend them to avoid dead loop.
> +
> +== more description of 'DataObject' type ==
> +
> +We use 'DataObject' to describe dynamical data struct, it might be
> +nested dictionary, list or string.
> +
> +'DataObject' itself is a arbitrary and nested dictionary, the
> +dictionary has three keys ('key', 'type', 'data'), 'key' and
> +'data' are optional.
> +
> +* For describing Dictionary, we set the key to 'key', and set the
> + value to 'type'
> +* For describing List, we don't set 'key', just set the value to
> + 'type'
> +* If the value of dictionary or list is non-native type, we extend
> + the non-native type to dictionary, set it to 'data', and set the
> + non-native type's name to 'type'.
> +* If the value of dictionary or list is dictionary or list, 'type'
> + won't be set.
> +
> +== examples ==
> +
> +1) Dict, value is native type
> +{ 'id': 'str', ... }
> +--------------------
> +[
> + {
> + "key": "id",
> + "type": "str"
> + },
> + .....
> +]
> +
> +2) Dict, value is defined types
> +{ 'options': 'TpmTypeOptions' }
> +-------------------------------
> +[
> + {
> + "key": "options",
> + "type": "TpmTypeOptions",
> + "data": [
> + {
> + "key": "passthrough",
> + "type": "str",
> + }
> + ]
> + },
> + .....
> +]
> +
> +3) List, value is native type
> +['str', ... ]
> +-------------
> +[
> + {
> + "type": "str"
> + },
> + ....
> +]
> +
> +4) List, value is defined types
> +['TpmTypeOptions', ... ]
> +------------------------
> +[
> + {
> + "type": "TpmTypeOptions",
> + "data": [
> + {
> + "key": "passthrough",
> + "type": "str",
> + }
> + ]
> + },
> + .....
> +]
> +
> +5) Dict, value is dictionary
> +{ 'info': { 'age': 'init', ... } }
> +-----------------------------
> +[
> + {
> + "key": "info",
> + "data": [
> + {
> + "key": "age",
> + "type": "init",
> + },
> + ...
> + ]
> + },
> +]
> +
> +6) Dict, value is list
> +{ 'info': [ 'str', ... } }
> +-----------------------------
> +[
> + {
> + "key": "info",
> + "data": [
> + {
> + "type": "str",
> + },
> + ...
> + ]
> + },
> +]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7b9fef1..cf03391 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3679,3 +3679,72 @@
> '*cpuid-input-ecx': 'int',
> 'cpuid-register': 'X86CPURegister32',
> 'features': 'int' } }
> +
> +##
> +# @DataObject
> +#
> +# Details of a data object, it can be nested dictionary/list
> +#
> +# @key: #optional Data object key
> +#
> +# @type: Data object type name
> +#
> +# @optional: #optional whether the data object is optional
> +#
> +# @data: #optional DataObject list, can be a dictionary or list type
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'DataObject',
> + 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
> +
> +##
> +# @SchemaMetaType
> +#
> +# Possible meta types of a schema entry
> +#
> +# @Command: QMP command
> +#
> +# @Type: defined new data type
> +#
> +# @Enumeration: enumeration data type
> +#
> +# @Union: union data type
> +#
> +# Since: 1.6
> +##
> +{ 'enum': 'SchemaMetaType',
> + 'data': ['Command', 'Type', 'Enumeration', 'Union'] }
> +
> +##
> +# @SchemaEntry
> +#
> +# Details of schema items
> +#
> +# @type: Entry's type in string format
It's not a string, it's a SchemaMetaType.
> +#
> +# @name: Entry name
> +#
> +# @data: #optional list of DataObject. This can have different meaning
> +# depending on the 'type' value. For example, for a QMP command,
> +# this member contains an argument listing. For an enumeration,
> +# it contains the enum's values and so on
> +#
> +# @returns: #optional list of DataObject, return data after executing
> +# QMP command
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
> + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e075df4..e3cbe93 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3047,3 +3047,42 @@ Example:
> <- { "return": {} }
>
> 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', 'event'
> +- "data": schema data (json-object, optional)
> +- "returns": return data of qmp command (json-object, optional)
> +
> +Example:
> +
> +-> { "execute": "query-qmp-schema" }
> +<- { "return": [
> + {
> + "name": "query-name",
> + "type": "Command",
> + "returns": [
> + {
> + "key": "*name",
> + "type": "str"
> + }
> + ]
> + }
> + ]
> + }
> +
> +EQMP
> diff --git a/qmp.c b/qmp.c
> index 4c149b3..3ace3a6 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,311 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> return arch_query_cpu_definitions(errp);
> }
>
> +/*
> + * Use a string to record the visit path, type index of each node
> + * will be saved to the string, indexes are split by ':'.
> + */
> +static char visit_path_str[1024];
visit path? I don't get this.
> +
> +/* push the type index to 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 DataObjectList *visit_qobj_dict(QObject *data);
> +static DataObjectList *visit_qobj_list(QObject *data);
> +
> +/* extend defined type to json object */
> +static DataObjectList *extend_type(const char* str)
> +{
> + DataObjectList *data_list;
> + QObject *data;
> + QDict *qdict;
> + const QDictEntry *ent;
> + int i;
> +
> + /* don't extend builtin types */
> + 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")) {
> + return NULL;
> + }
> +
> + for (i = 0; qmp_schema_table[i]; i++) {
> + data = qobject_from_json(qmp_schema_table[i]);
> + assert(data != NULL);
> +
> + qdict = qobject_to_qdict(data);
> + assert(qdict != NULL);
> +
> + ent = qdict_first(qdict);
> + if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
> + && !qdict_get(qdict, "union")) {
> + continue;
> + }
> +
> + if (!strcmp(str, qstring_copy_str(ent->value))) {
> + 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);
> +
> + data = qdict_get(qdict, "data");
> + if(data) {
> + if (data->type->code == QTYPE_QDICT) {
> + data_list = visit_qobj_dict(data);
> + } else if (data->type->code == QTYPE_QLIST) {
> + data_list = visit_qobj_list(data);
> + }
> + /* pop index from visit_path_str after extending */
> + pop_id();
> +
> + return data_list;
> + }
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static DataObjectList *visit_qobj_list(QObject *data)
> +{
> + DataObjectList *obj_list, *obj_last_entry, *obj_entry;
> + DataObject *obj_info;
> + const QListEntry *ent;
> + QList *qlist;
> +
> + qlist = qobject_to_qlist(data);
> + assert(qlist != NULL);
> +
> + obj_list = NULL;
> + for (ent = qlist_first(qlist); ent; ent = qlist_next(ent)) {
> + obj_info = g_malloc0(sizeof(*obj_info));
> + obj_entry = g_malloc0(sizeof(*obj_entry));
> + obj_entry->value = obj_info;
> + obj_entry->next = NULL;
> +
> + if (!obj_list) {
> + obj_list = obj_entry;
> + } else {
> + obj_last_entry->next = obj_entry;
> + }
> + obj_last_entry = obj_entry;
> +
> + if (ent->value->type->code == QTYPE_QDICT) {
> + obj_info->data = visit_qobj_dict(ent->value);
> + } else if (ent->value->type->code == QTYPE_QLIST) {
> + obj_info->data = visit_qobj_list(ent->value);
> + } else if (ent->value->type->code == QTYPE_QSTRING) {
> + obj_info->has_type = true;
> + obj_info->type = g_strdup(qstring_copy_str(ent->value));
> + obj_info->data = extend_type(qstring_copy_str(ent->value));
> + }
> + if (obj_info->data) {
> + obj_info->has_data = true;
> + }
> + }
> +
> + return obj_list;
> +}
> +
> +static DataObjectList *visit_qobj_dict(QObject *data)
> +{
> + DataObjectList *obj_list, *obj_last_entry, *obj_entry;
> + DataObject *obj_info;
> + const QDictEntry *ent;
> + QDict *qdict;
> +
> + qdict = qobject_to_qdict(data);
> + assert(qdict != NULL);
> +
> + obj_list = NULL;
> + for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
> + obj_info = g_malloc0(sizeof(*obj_info));
> + obj_entry = g_malloc0(sizeof(*obj_entry));
> + obj_entry->value = obj_info;
> + obj_entry->next = NULL;
> +
> + if (!obj_list) {
> + obj_list = obj_entry;
> + } else {
> + obj_last_entry->next = obj_entry;
> + }
> + obj_last_entry = obj_entry;
> +
> + if (ent->key[0] == '*') {
> + obj_info->key = g_strdup(ent->key + 1);
> + obj_info->has_optional = true;
> + obj_info->optional = true;
> + } else {
> + obj_info->key = g_strdup(ent->key);
> + }
> + obj_info->has_key = true;
> +
> + if (ent->value->type->code == QTYPE_QDICT) {
> + obj_info->data = visit_qobj_dict(ent->value);
> + } else if (ent->value->type->code == QTYPE_QLIST) {
> + obj_info->data = visit_qobj_list(ent->value);
> + } else if (ent->value->type->code == QTYPE_QSTRING) {
> + obj_info->has_type = true;
> + obj_info->type = g_strdup(qstring_copy_str(ent->value));
> + obj_info->data = extend_type(qstring_copy_str(ent->value));
> + }
> + if (obj_info->data) {
> + obj_info->has_data = true;
> + }
> + }
> +
> + return obj_list;
> +}
> +
> +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> +{
> + SchemaEntryList *list, *last_entry, *entry;
> + SchemaEntry *info;
> + DataObjectList *obj_entry;
> + DataObject *obj_info;
> + QObject *data;
> + QDict *qdict;
> + int i;
> +
> + list = NULL;
> + for (i = 0; qmp_schema_table[i]; i++) {
> + data = qobject_from_json(qmp_schema_table[i]);
> + assert(data != NULL);
> +
> + qdict = qobject_to_qdict(data);
> + assert(qdict != NULL);
> +
> + if (qdict_get(qdict, "command")) {
> + info = g_malloc0(sizeof(*info));
> + info->type = SCHEMA_META_TYPE_COMMAND;
> + info->name = strdup(qdict_get_str(qdict, "command"));
s/strdup/g_strdup
> + } else {
> + continue;
> + }
You return only commands. That is, types are returned as part of the
command input. ErrorClass can't be queried then? I'm not judging, only
observing.
Eric, this is good enough for libvirt?
Btw, you're leaking data on the else clause.
> +
> + memset(visit_path_str, 0, sizeof(visit_path_str));
> + data = qdict_get(qdict, "data");
> + if (data) {
> + info->has_data = true;
> + if (data->type->code == QTYPE_QLIST) {
> + info->data = visit_qobj_list(data);
> + } else if (data->type->code == QTYPE_QDICT) {
> + info->data = visit_qobj_dict(data);
> + } else if (data->type->code == QTYPE_QSTRING) {
> + info->data = extend_type(qstring_get_str(qobject_to_qstring(data)));
> + if (!info->data) {
> + obj_info = g_malloc0(sizeof(*obj_info));
> + obj_entry = g_malloc0(sizeof(*obj_entry));
> + obj_entry->value = obj_info;
> + obj_info->has_type = true;
> + obj_info->type = g_strdup(qdict_get_str(qdict, "data"));
> + info->data = obj_entry;
> + }
> + } else {
> + abort();
Pleae, explain why you're aborting here.
> + }
> + }
> +
> + memset(visit_path_str, 0, sizeof(visit_path_str));
> + data = qdict_get(qdict, "returns");
> + if (data) {
> + info->has_returns = true;
> + if (data->type->code == QTYPE_QLIST) {
> + info->returns = visit_qobj_list(data);
> + } else if (data->type->code == QTYPE_QDICT) {
> + info->returns = visit_qobj_dict(data);
> + } else if (data->type->code == QTYPE_QSTRING) {
> + info->returns = extend_type(qstring_copy_str(data));
> + if (!info->returns) {
> + obj_info = g_malloc0(sizeof(*obj_info));
> + obj_entry = g_malloc0(sizeof(*obj_entry));
> + obj_entry->value = obj_info;
> + obj_info->has_type = true;
> + obj_info->type = g_strdup(qdict_get_str(qdict, "returns"));
> + info->returns = obj_entry;
> + }
> + }
> + }
> +
> + entry = g_malloc0(sizeof(DataObjectList *));
> + entry->value = info;
> + 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)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json Amos Kong
2013-07-17 20:09 ` Luiz Capitulino
@ 2013-07-19 12:27 ` Eric Blake
2013-07-26 6:53 ` Amos Kong
1 sibling, 1 reply; 26+ messages in thread
From: Eric Blake @ 2013-07-19 12:27 UTC (permalink / raw)
To: Amos Kong; +Cc: pbonzini, aliguori, lcapitulino, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 3677 bytes --]
On 07/16/2013 04:37 AM, 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.
>
> eg:
> 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> scripts/qapi-visit.py | 2 +-
> scripts/qapi.py | 4 +++-
> 5 files changed, 53 insertions(+), 7 deletions(-)
My python is weak, but I'll attempt a review anyway (how else do you
learn a new language? :)
> @@ -223,6 +223,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 "." -i "$@" < $<, " GEN $@")
Copy-and-paste, but any reason there is so much space around GEN?
> -exprs = parse_schema(sys.stdin)
> +exprs_all = parse_schema(sys.stdin)
> +
> +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
Is it worth stating what file this was generated from? If I open a
generated file to try and make an edit, I like to have it tell me what
the real source file is.
> +
> +/*
> + * Schema json string table converted from qapi-schema.json
Well, this is close, but as we are asking you to do multiple
qapi-schema.json files (one for qemu's QMP monitor, one for qemu-ga), a
relative path to the file within the overall qemu.git might be nicer.
> + *
> + * Copyright (c) 2013 Red Hat, Inc.
This copyright won't auto-update as years change. Should it?
Then again, this is probably copy-and-paste from other files the
generator already spits out, so cleanups to one generated header should
probably done to all generated headers, and in a separate cleanup patch,
if at all.
> + *
> + * 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 introspect_file:
> + for line in exprs_all[1]:
> + line = re.sub(r'\n', ' ', line.strip())
> + line = re.sub(r' +', ' ', line)
> + schema_table += ' "%s",\n' % (line)
Do we ever need to worry about someone using { "command" ...} instead of
the current style of { 'command' ...} in the qapi-schema.json file? If
they do, then you would be generating invalid C code here by not
escaping the " properly. Likewise, should you be asserting that there
are no other problematic characters like backslash? Ideally, we will
never encounter such problems, but being robust might save some
head-scratching if someone introduces a typo.
I can live with what you have:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-17 20:36 ` Luiz Capitulino
@ 2013-07-19 20:26 ` Eric Blake
2013-07-26 7:21 ` Amos Kong
1 sibling, 0 replies; 26+ messages in thread
From: Eric Blake @ 2013-07-19 20:26 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: pbonzini, aliguori, Amos Kong, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]
On 07/17/2013 02:36 PM, Luiz Capitulino wrote:
>> We need to parse all commands json definition, and generated a
>> dynamical tree, QMP infrastructure will convert the tree to
>> json string and return to QMP client.
>>
>> So here I defined a 'DataObject' type in qapi-schema.json,
>> it's used to describe the dynamical dictionary/list/string.
>
> I skimmed over the patch and made some comments, but my impression
> is that this is getting too complex... Why did we move from letting
> mngt query type by type (last version) to this version which returns
> all commands and their input types? Does this satisfy libvirt needs?
Libvirt can query once, and then browse through the (giant) reply as
many times as needed to drill down to a full definition. The ability to
filter by passing in a name to look up is a bonus, but not mandatory.
I'm also working on a reply to the full patch.
> You return only commands. That is, types are returned as part of the
> command input. ErrorClass can't be queried then? I'm not judging, only
> observing.
>
> Eric, this is good enough for libvirt?
It might be sufficient (after all, you can't use a type except by
passing it as part of a command [argument or return] or event [which we
still don't have converted into qapi...]). In one respect, it means
that if we want to know if an optional field was added for command
'foo', we start by querying command foo; then regardless of what type
names were used, we will see that in the results for the command. On
the other hand, we've been arguing that qapi type names are part of the
API, and that we shouldn't arbitrarily change type names (particularly
not once introspection is in place). Thus, if I can query for the
contents of type 'FooDict', that shaves a step from querying the
structure of command 'foo' that uses the type 'FooDict'.
In other words, it will "work" for libvirt, but it may not be optimal.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
2013-07-16 10:48 ` Paolo Bonzini
2013-07-17 20:36 ` Luiz Capitulino
@ 2013-07-19 22:05 ` Eric Blake
2013-07-26 7:51 ` Amos Kong
2013-11-27 2:32 ` Amos Kong
2 siblings, 2 replies; 26+ messages in thread
From: Eric Blake @ 2013-07-19 22:05 UTC (permalink / raw)
To: Amos Kong; +Cc: kwolf, aliguori, armbru, qemu-devel, lcapitulino, pbonzini
[-- Attachment #1: Type: text/plain, Size: 13690 bytes --]
On 07/16/2013 04:37 AM, Amos Kong wrote:
> Introduces new monitor command to query QMP schema information,
> the return data is a dynamical and nested dict/list, it contains
s/dynamical/dynamic/
> the useful metadata to help management to check feature support,
> QMP commands detail, etc.
>
> I added a document for QMP introspection support.
> (docs/qmp-full-introspection.txt)
Yay - docs make a world of difference.
>
> We need to parse all commands json definition, and generated a
mixing tense ("need" present, "generated" past); for commit messages,
present tense is generally appropriate. Thus:
s/generated/generate/
> dynamical tree, QMP infrastructure will convert the tree to
s/dynamical/dynamic/
> json string and return to QMP client.
>
> So here I defined a 'DataObject' type in qapi-schema.json,
> it's used to describe the dynamical dictionary/list/string.
s/dynamical/dynamic/
>
> { 'type': 'DataObject',
> 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
>
> Not all the keys in data will be used.
> # List: type
> # Dict: key, type
> # nested List: type, data
> # nested Dict: key, type, data
I wonder if we can take advantage of Kevin's work on unions to make this
MUCH easier to use:
https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html
{ 'enum': 'DataObjectType',
'data': [ 'command', 'struct', 'union', 'enum' ] }
# extend to add 'event' later
{ 'type': 'DataObjectBase',
'data': { 'name': 'str' } }
{ 'union': 'DataObjectMemberType',
'discriminator': {},
'data': { 'reference': 'str',
'inline': 'DataObject' } }
{ 'type': DataObjectMember',
'data': { 'name': 'str', 'type': 'DataObjectMemberType',
'*optional': 'bool', '*recursive': 'bool' } }
{ 'type': 'DataObjectStruct',
'data': { 'data': [ 'DataObjectMember' ] } }
{ 'type': 'DataObjectEnum',
'data': { 'data': [ 'str' ] } }
{ 'type': 'DataObjectUnion',
'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
'*discriminator': 'str' } }
{ 'type': 'DataObjectCommand',
'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'type',
'data': {
'struct': 'DataObjectStruct',
'enum': 'DataObjectEnum',
'union': 'DataObjectUnion',
'command': 'DataObjectCommand',
'array': {} }
>
> The DataObject is described in docs/qmp-full-introspection.txt in
> detail.
>
> The following content gives an example of query-tpm-types:
>
> ## Define example in qapi-schema.json:
>
> { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
It might be more useful to have a (made-up) example that shows as many
details as possible - a command that takes arguments and has a return
type will exercise more of the code paths than a query command with only
a return.
>
> ## Returned description:
>
> {
> "name": "query-tpm-types",
> "type": "Command",
> "returns": [
> {
> "type": "TpmType",
> "data": [
> {
> "type": "passthrough"
> }
> ]
I need a way to know the difference between a TpmType returned directly
in a dict, vs. a return containing an array of TpmType.
> }
> ]
> },
Thus, using the discriminated union I set out above, I would return
something like:
{ "name": "TpmType", "type": "enum",
"data": [ "passthrough" ] },
{ "name": "query-tpm-types", "type": "command",
"data": [ ],
"returns": { "name": "TpmType", "type": "array" } }
>
> 'TpmType' is a defined type, it will be extended in returned
> description. [ 'passthrough' ] is a list, so 'type' and 'data'
> will be used.
>
> TODO:
> We can add events definations to qapi-schema.json by another
s/definations/definitions/
> patch, then event can also be queried.
>
> Introduce another command 'query-qga-schema' to query QGA schema
> information, it's easy to add this support with current current
> patch.
Such a command would be part of the QGA interface, not a QMP command.
But yes, it is needed, and the ideal patch series from you will include
that patch as part of the series. But I don't mind waiting until we get
the interface nailed down before you actually implement the QGA repeat
of the interface.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
> qapi-schema.json | 69 +++++++++
> qmp-commands.hx | 39 +++++
> qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 558 insertions(+)
> create mode 100644 docs/qmp-full-introspection.txt
>
> diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> new file mode 100644
> index 0000000..cc0fb80
> --- /dev/null
> +++ b/docs/qmp-full-introspection.txt
> @@ -0,0 +1,143 @@
> += full introspection support for QMP =
> +
Is it worth merging this into the existing qapi-code-gen.txt, or at
least having the two documents refer to one another, since they deal
with related concepts (turning the .json file into generated code)?
> +== Purpose ==
> +
> +Add a new interface to provide QMP schema information to management,
s/a new/an/ - after a year, the interface won't be new, but this doc
will still be relevant.
> +the return data is a dynamical and nested dict/list, it contains
s/dynamical/dynamic/
> +the useful metadata to help management to check feature support,
> +QMP commands detail, etc.
> +
> +== Usage ==
> +
> +Execute QMP command:
> +
> + { "execute": "query-qmp-schema" }
> +
> +Returns:
> +
> + { "return": [
> + {
> + "name": "query-name",
> + "type": "Command",
> + "returns": [
> + {
> + "key": "*name",
> + "type": "str"
> + }
> + ]
Are we trying to use struct names where they exist for compactness, or
trying to inline-expand everything as far as possible until we run into
self-referential recursion?
> + },
> + ...
> + }
> +
> +The whole schema information will be returned in one go, it contains
> +commands and event. It doesn't support to be filtered by type or name.
s/event/events/
s/It/At present, it/
s/to be filtered/filtering/
> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
This list will get out of date quickly. I'd just word it generically:
We have several self-referential types
> +that uses themself in their own define data directly or indirectly,
s/uses themself/use themselves/
> +we will not repeatedly extend them to avoid dead loop.
Would it be worth a flag in the QMP output when a type is not being
further expanded because it is a nested occurrence of itself? That is,
given my proposed layout above, ImageInfo would turn into something like:
{ "name": "ImageInfo", "type": "struct",
"data": [ { "name": "filename", "type", "str" },
...
{ "name": "backing-image", "type": "ImageInfo",
"optional": true, "recursive": true } ] },
> +
> +== more description of 'DataObject' type ==
> +
> +We use 'DataObject' to describe dynamical data struct, it might be
s/dynamical/dynamic/
> +nested dictionary, list or string.
> +
> +'DataObject' itself is a arbitrary and nested dictionary, the
> +dictionary has three keys ('key', 'type', 'data'), 'key' and
spacing is odd.
> +'data' are optional.
> +
> +* For describing Dictionary, we set the key to 'key', and set the
> + value to 'type'
> +* For describing List, we don't set 'key', just set the value to
> + 'type'
Again, if you use the idea of a discriminated union, you don't need
quite the complexity in describing this: dictionaries are listed with
key/type/optional tuples, lists (enums) are listed with just an array of
strings, and the QAPI perfectly described that difference by the
discriminator telling you 'struct' vs. 'enum'.
> +* If the value of dictionary or list is non-native type, we extend
> + the non-native type to dictionary, set it to 'data', and set the
> + non-native type's name to 'type'.
Again, I tried to set up the QAPI that describes something that uses an
anonymous union that either gives a string (the name of a primitive or
already-defined type) or a struct (a recursion into another layer of
struct describing the structure of that type in line).
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7b9fef1..cf03391 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3679,3 +3679,72 @@
> '*cpuid-input-ecx': 'int',
> 'cpuid-register': 'X86CPURegister32',
> 'features': 'int' } }
> +
> +##
> +# @DataObject
> +#
> +# Details of a data object, it can be nested dictionary/list
> +#
> +# @key: #optional Data object key
> +#
> +# @type: Data object type name
> +#
> +# @optional: #optional whether the data object is optional
mention that the default is false.
> +#
> +# @data: #optional DataObject list, can be a dictionary or list type
so if 'data' is present, we are describing @type in more detail; if it
is absent, then @type is primitive.
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'DataObject',
> + 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
> +
'*type' should be an enum type, not open-coded string.
> +##
> +# @SchemaMetaType
> +#
> +# Possible meta types of a schema entry
> +#
> +# @Command: QMP command
> +#
> +# @Type: defined new data type
> +#
> +# @Enumeration: enumeration data type
> +#
> +# @Union: union data type
> +#
> +# Since: 1.6
> +##
> +{ 'enum': 'SchemaMetaType',
> + 'data': ['Command', 'Type', 'Enumeration', 'Union'] }
Do we need to start these with a capital? On the other hand, having
them as capitals may make it easier to ensure we are outputting correct
information.
> +
> +##
> +# @SchemaEntry
> +#
> +# Details of schema items
> +#
> +# @type: Entry's type in string format
> +#
> +# @name: Entry name
> +#
> +# @data: #optional list of DataObject. This can have different meaning
> +# depending on the 'type' value. For example, for a QMP command,
> +# this member contains an argument listing. For an enumeration,
> +# it contains the enum's values and so on
This argues for making it a union type.
> +#
> +# @returns: #optional list of DataObject, return data after executing
> +# QMP command
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
> + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
If you don't take any arguments, then the "returns an error" statement
is impossible.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e075df4..e3cbe93 100644
> --- a/qmp-commands.hx
>
> +/*
> + * Use a string to record the visit path, type index of each node
> + * will be saved to the string, indexes are split by ':'.
> + */
> +static char visit_path_str[1024];
Is a fixed width buffer really the best solution, or does glib offer us
something better? For example, a hash table, or even a growable string.
> +
> + for (i = 0; qmp_schema_table[i]; i++) {
> + data = qobject_from_json(qmp_schema_table[i]);
> + assert(data != NULL);
> +
> + qdict = qobject_to_qdict(data);
> + assert(qdict != NULL);
> +
> + ent = qdict_first(qdict);
> + if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
> + && !qdict_get(qdict, "union")) {
> + continue;
> + }
Why are we doing the work in C code, every time the command is called?
Wouldn't it be more efficient to do the work in python code, at the time
the qmp_schema_table C code is first generated, so that the generated
code is already in the right format?
> +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> +{
> + SchemaEntryList *list, *last_entry, *entry;
> + SchemaEntry *info;
> + DataObjectList *obj_entry;
> + DataObject *obj_info;
> + QObject *data;
> + QDict *qdict;
> + int i;
> +
> + list = NULL;
> + for (i = 0; qmp_schema_table[i]; i++) {
> + data = qobject_from_json(qmp_schema_table[i]);
> + assert(data != NULL);
> +
> + qdict = qobject_to_qdict(data);
> + assert(qdict != NULL);
> +
> + if (qdict_get(qdict, "command")) {
> + info = g_malloc0(sizeof(*info));
> + info->type = SCHEMA_META_TYPE_COMMAND;
> + info->name = strdup(qdict_get_str(qdict, "command"));
> + } else {
> + continue;
> + }
> +
Don't we want to also output types (struct, union, enum) and eventually
events, not just commands?
I hope we're converging, but I'm starting to worry that we won't have a
design in place in time for the 1.6 release.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json
2013-07-17 20:09 ` Luiz Capitulino
@ 2013-07-26 3:39 ` Amos Kong
0 siblings, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-26 3:39 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: pbonzini, aliguori, qemu-devel, armbru
On Wed, Jul 17, 2013 at 04:09:32PM -0400, Luiz Capitulino wrote:
> On Tue, 16 Jul 2013 18:37:41 +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.
> >
> > eg:
> > 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > scripts/qapi-visit.py | 2 +-
> > scripts/qapi.py | 4 +++-
> > 5 files changed, 53 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c06bfab..2348bce 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -38,7 +38,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
> > @@ -223,6 +223,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 "." -i "$@" < $<, " GEN $@")
>
> What about qemu-ga?
We hadn't confirm with this implement way. It's easy to add a new
command to query qga schema info. I can do it in next version.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json
2013-07-19 12:27 ` Eric Blake
@ 2013-07-26 6:53 ` Amos Kong
0 siblings, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-26 6:53 UTC (permalink / raw)
To: Eric Blake; +Cc: pbonzini, aliguori, lcapitulino, qemu-devel, armbru
On Fri, Jul 19, 2013 at 06:27:12AM -0600, Eric Blake wrote:
> On 07/16/2013 04:37 AM, 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.
> >
> > eg:
> > 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++---
> > scripts/qapi-visit.py | 2 +-
> > scripts/qapi.py | 4 +++-
> > 5 files changed, 53 insertions(+), 7 deletions(-)
>
> My python is weak, but I'll attempt a review anyway (how else do you
> learn a new language? :)
>
> > @@ -223,6 +223,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 "." -i "$@" < $<, " GEN $@")
>
> Copy-and-paste, but any reason there is so much space around GEN?
>
> > -exprs = parse_schema(sys.stdin)
> > +exprs_all = parse_schema(sys.stdin)
> > +
> > +schema_table = """/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>
> Is it worth stating what file this was generated from? If I open a
> generated file to try and make an edit, I like to have it tell me what
> the real source file is.
>
> > +
> > +/*
> > + * Schema json string table converted from qapi-schema.json
>
> Well, this is close, but as we are asking you to do multiple
> qapi-schema.json files (one for qemu's QMP monitor, one for qemu-ga), a
> relative path to the file within the overall qemu.git might be nicer.
I will use another split file qga-schema.h for qemu-ga.
>
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc.
>
> This copyright won't auto-update as years change. Should it?
>
> Then again, this is probably copy-and-paste from other files the
> generator already spits out, so cleanups to one generated header should
> probably done to all generated headers, and in a separate cleanup patch,
> if at all.
>
> > + *
> > + * 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 introspect_file:
> > + for line in exprs_all[1]:
> > + line = re.sub(r'\n', ' ', line.strip())
> > + line = re.sub(r' +', ' ', line)
> > + schema_table += ' "%s",\n' % (line)
>
> Do we ever need to worry about someone using { "command" ...} instead of
> the current style of { 'command' ...} in the qapi-schema.json file? If
> they do, then you would be generating invalid C code here by not
> escaping the " properly.
We didn't allow to use " in qapi-schema.json, you can check scripts/qapi.py:tokenize().
> Likewise, should you be asserting that there
> are no other problematic characters like backslash? Ideally, we will
> never encounter such problems, but being robust might save some
> head-scratching if someone introduces a typo.
>
> I can live with what you have:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-16 12:18 ` Paolo Bonzini
@ 2013-07-26 7:03 ` Amos Kong
0 siblings, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-26 7:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: armbru, aliguori, qemu-devel, lcapitulino
On Tue, Jul 16, 2013 at 02:18:37PM +0200, Paolo Bonzini wrote:
> Il 16/07/2013 14:04, Amos Kong ha scritto:
> >> > Thanks. I see this is unique, but it is also not too intuitive.
> >> >
> >> > So, could you add a "kind" field to DataObject that is an enum
> >> > (list/dict/scalar, or something like that)? This would make it easier
> >> > to parse (for humans at least, but I guess also for programs).
> > I thought we can identify the kind by some judgment.
>
> Yes, I understood that. Strictly speaking the kind is redundant, but it
> seems to me that it makes the API easier to understand and use.
>
> > if the dict has key 'key', it's a dict
> > if no 'key', have 'type', it's a list
> > if only have 'type', it's a buildin type (or extended type that
> > doesn't need to be extended)
> > if no 'key', have 'type' & 'data', it's extended list type
> > if have 'key', 'type', 'data', it's extended dict type
> >
> > I will added a 'kind' field to make it clearer.
> >
> > KIND enum:
> > list
> > dict
> > str
>
> Why "str" and not "scalar" for a builtin type? It's not necessarily a
> string, is it?
right, 'scalar' is better.
> Paolo
>
> > scalar(bool): Or just simplely check if have 'data' key?
> > true/false
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-17 20:36 ` Luiz Capitulino
2013-07-19 20:26 ` Eric Blake
@ 2013-07-26 7:21 ` Amos Kong
1 sibling, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-07-26 7:21 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: pbonzini, aliguori, qemu-devel, armbru
On Wed, Jul 17, 2013 at 04:36:06PM -0400, Luiz Capitulino wrote:
> On Tue, 16 Jul 2013 18:37:42 +0800
> Amos Kong <akong@redhat.com> wrote:
>
> > Introduces new monitor command to query QMP schema information,
> > the return data is a dynamical and nested dict/list, it contains
> > the useful metadata to help management to check feature support,
> > QMP commands detail, etc.
> >
> > I added a document for QMP introspection support.
> > (docs/qmp-full-introspection.txt)
> >
> > We need to parse all commands json definition, and generated a
> > dynamical tree, QMP infrastructure will convert the tree to
> > json string and return to QMP client.
> >
> > So here I defined a 'DataObject' type in qapi-schema.json,
> > it's used to describe the dynamical dictionary/list/string.
>
> I skimmed over the patch and made some comments, but my impression
> is that this is getting too complex... Why did we move from letting
> mngt query type by type (last version) to this version which returns
> all commands and their input types? Does this satisfy libvirt needs?
> > diff --git a/qmp.c b/qmp.c
> > index 4c149b3..3ace3a6 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,311 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> > return arch_query_cpu_definitions(errp);
> > }
> >
> > +/*
> > + * Use a string to record the visit path, type index of each node
> > + * will be saved to the string, indexes are split by ':'.
> > + */
> > +static char visit_path_str[1024];
>
> visit path? I don't get this.
The return data is a dynamic tree. When we found defined type, we need to extend it.
But some defined type is used in its child's node. So I use a visit_path_str to
record the type index in one node. We don't extend one type, if it's already
been extened in parent's node.
{'type': 'a', 'data': ['a', 'b', 'c']}
{'command': 'cmd', 'data': 'a'}
'a' is a defined type, we will extended it. But we will not extend 'a'
in data list of type 'a'.
I will add the explain to the doc.
> > +
> > +/* push the type index to 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';
> > + }
> > +}
> > +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> > +{
> > + SchemaEntryList *list, *last_entry, *entry;
> > + SchemaEntry *info;
> > + DataObjectList *obj_entry;
> > + DataObject *obj_info;
> > + QObject *data;
> > + QDict *qdict;
> > + int i;
> > +
> > + list = NULL;
> > + for (i = 0; qmp_schema_table[i]; i++) {
> > + data = qobject_from_json(qmp_schema_table[i]);
> > + assert(data != NULL);
> > +
> > + qdict = qobject_to_qdict(data);
> > + assert(qdict != NULL);
> > +
> > + if (qdict_get(qdict, "command")) {
> > + info = g_malloc0(sizeof(*info));
> > + info->type = SCHEMA_META_TYPE_COMMAND;
> > + info->name = strdup(qdict_get_str(qdict, "command"));
>
> s/strdup/g_strdup
>
> > + } else {
> > + continue;
> > + }
>
> You return only commands. That is, types are returned as part of the
> command input.
Right.
> ErrorClass can't be queried then? I'm not judging, only
> observing.
We can return the 'type', 'enum', 'union', 'etc' if libvirt needs them.
> Eric, this is good enough for libvirt?
>
> Btw, you're leaking data on the else clause.
>
> > +
> > + memset(visit_path_str, 0, sizeof(visit_path_str));
> > + data = qdict_get(qdict, "data");
> > + if (data) {
> > + info->has_data = true;
> > + if (data->type->code == QTYPE_QLIST) {
> > + info->data = visit_qobj_list(data);
> > + } else if (data->type->code == QTYPE_QDICT) {
> > + info->data = visit_qobj_dict(data);
> > + } else if (data->type->code == QTYPE_QSTRING) {
> > + info->data = extend_type(qstring_get_str(qobject_to_qstring(data)));
> > + if (!info->data) {
> > + obj_info = g_malloc0(sizeof(*obj_info));
> > + obj_entry = g_malloc0(sizeof(*obj_entry));
> > + obj_entry->value = obj_info;
> > + obj_info->has_type = true;
> > + obj_info->type = g_strdup(qdict_get_str(qdict, "data"));
> > + info->data = obj_entry;
> > + }
> > + } else {
> > + abort();
>
> Pleae, explain why you're aborting here.
{ 'command': 'query-qmp-schema', 'data': XXXX }
the type of XXXX can only be list, dictionary or buildin-type.
XXXX is the value in define dictionary.
> > + }
> > + }
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-19 22:05 ` Eric Blake
@ 2013-07-26 7:51 ` Amos Kong
2013-07-26 11:52 ` Eric Blake
2013-11-27 2:32 ` Amos Kong
1 sibling, 1 reply; 26+ messages in thread
From: Amos Kong @ 2013-07-26 7:51 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, aliguori, armbru, qemu-devel, lcapitulino, pbonzini
On Fri, Jul 19, 2013 at 04:05:16PM -0600, Eric Blake wrote:
> On 07/16/2013 04:37 AM, Amos Kong wrote:
> > Introduces new monitor command to query QMP schema information,
> > the return data is a dynamical and nested dict/list, it contains
>
> s/dynamical/dynamic/
>
> > the useful metadata to help management to check feature support,
> > QMP commands detail, etc.
> >
> > I added a document for QMP introspection support.
> > (docs/qmp-full-introspection.txt)
>
> Yay - docs make a world of difference.
>
> >
> > We need to parse all commands json definition, and generated a
>
> mixing tense ("need" present, "generated" past); for commit messages,
> present tense is generally appropriate. Thus:
>
> s/generated/generate/
>
> > dynamical tree, QMP infrastructure will convert the tree to
>
> s/dynamical/dynamic/
>
> > json string and return to QMP client.
> >
> > So here I defined a 'DataObject' type in qapi-schema.json,
> > it's used to describe the dynamical dictionary/list/string.
>
> s/dynamical/dynamic/
>
> >
> > { 'type': 'DataObject',
> > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> >
> > Not all the keys in data will be used.
> > # List: type
> > # Dict: key, type
> > # nested List: type, data
> > # nested Dict: key, type, data
>
> I wonder if we can take advantage of Kevin's work on unions to make this
> MUCH easier to use:
>
> https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html
>
> { 'enum': 'DataObjectType',
> 'data': [ 'command', 'struct', 'union', 'enum' ] }
It's necessary!!
> # extend to add 'event' later
>
> { 'type': 'DataObjectBase',
> 'data': { 'name': 'str' } }
>
> { 'union': 'DataObjectMemberType',
> 'discriminator': {},
> 'data': { 'reference': 'str',
> 'inline': 'DataObject' } }
>
> { 'type': DataObjectMember',
> 'data': { 'name': 'str', 'type': 'DataObjectMemberType',
> '*optional': 'bool', '*recursive': 'bool' } }
>
> { 'type': 'DataObjectStruct',
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectEnum',
> 'data': { 'data': [ 'str' ] } }
> { 'type': 'DataObjectUnion',
> 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> '*discriminator': 'str' } }
> { 'type': 'DataObjectCommand',
> 'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }
Defining DataObject to a union, it will solve some hidden problem :)
> { 'union': 'DataObject',
> 'base': 'DataObjectBase',
> 'discriminator': 'type',
> 'data': {
> 'struct': 'DataObjectStruct',
> 'enum': 'DataObjectEnum',
> 'union': 'DataObjectUnion',
> 'command': 'DataObjectCommand',
> 'array': {} }
>
> >
> > The DataObject is described in docs/qmp-full-introspection.txt in
> > detail.
> >
> > The following content gives an example of query-tpm-types:
> >
> > ## Define example in qapi-schema.json:
> >
> > { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> > { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>
> It might be more useful to have a (made-up) example that shows as many
> details as possible - a command that takes arguments and has a return
> type will exercise more of the code paths than a query command with only
> a return.
OK.
> >
> > ## Returned description:
> >
> > {
> > "name": "query-tpm-types",
> > "type": "Command",
> > "returns": [
> > {
> > "type": "TpmType",
> > "data": [
> > {
> > "type": "passthrough"
> > }
> > ]
>
> I need a way to know the difference between a TpmType returned directly
> in a dict, vs. a return containing an array of TpmType.
QObject is always a dictionary, it can describe list and dictionary.
I will added a 'KIND' field for QObject.
> > }
> > ]
> > },
>
> Thus, using the discriminated union I set out above, I would return
> something like:
> { "name": "TpmType", "type": "enum",
> "data": [ "passthrough" ] },
> { "name": "query-tpm-types", "type": "command",
> "data": [ ],
> "returns": { "name": "TpmType", "type": "array" } }
>
> >
> > 'TpmType' is a defined type, it will be extended in returned
> > description. [ 'passthrough' ] is a list, so 'type' and 'data'
> > will be used.
> >
> > TODO:
> > We can add events definations to qapi-schema.json by another
>
> s/definations/definitions/
>
> > patch, then event can also be queried.
> >
> > Introduce another command 'query-qga-schema' to query QGA schema
> > information, it's easy to add this support with current current
> > patch.
>
> Such a command would be part of the QGA interface, not a QMP command.
> But yes, it is needed, and the ideal patch series from you will include
> that patch as part of the series. But I don't mind waiting until we get
> the interface nailed down before you actually implement the QGA repeat
> of the interface.
>
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
> > qapi-schema.json | 69 +++++++++
> > qmp-commands.hx | 39 +++++
> > qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 558 insertions(+)
> > create mode 100644 docs/qmp-full-introspection.txt
> >
> > diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> > new file mode 100644
> > index 0000000..cc0fb80
> > --- /dev/null
> > +++ b/docs/qmp-full-introspection.txt
> > @@ -0,0 +1,143 @@
> > += full introspection support for QMP =
> > +
>
> Is it worth merging this into the existing qapi-code-gen.txt, or at
> least having the two documents refer to one another, since they deal
> with related concepts (turning the .json file into generated code)?
This doc is not just about code generating (we just generated a simple
head file), most of work is about "full-introspection", the usage of
DataObject, implementation detail(visit path), examples.
> > +== Purpose ==
> > +
> > +Add a new interface to provide QMP schema information to management,
>
> s/a new/an/ - after a year, the interface won't be new, but this doc
> will still be relevant.
>
> > +the return data is a dynamical and nested dict/list, it contains
>
> s/dynamical/dynamic/
>
> > +the useful metadata to help management to check feature support,
> > +QMP commands detail, etc.
> > +
> > +== Usage ==
> > +
> > +Execute QMP command:
> > +
> > + { "execute": "query-qmp-schema" }
> > +
> > +Returns:
> > +
> > + { "return": [
> > + {
> > + "name": "query-name",
> > + "type": "Command",
> > + "returns": [
> > + {
> > + "key": "*name",
> > + "type": "str"
> > + }
> > + ]
>
> Are we trying to use struct names where they exist for compactness, or
> trying to inline-expand everything as far as possible until we run into
> self-referential recursion?
inline-expand everything
> > + },
> > + ...
> > + }
> > +
> > +The whole schema information will be returned in one go, it contains
> > +commands and event. It doesn't support to be filtered by type or name.
>
> s/event/events/
> s/It/At present, it/
> s/to be filtered/filtering/
>
> > +
> > +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
>
> This list will get out of date quickly. I'd just word it generically:
>
> We have several self-referential types
>
> > +that uses themself in their own define data directly or indirectly,
>
> s/uses themself/use themselves/
>
> > +we will not repeatedly extend them to avoid dead loop.
>
> Would it be worth a flag in the QMP output when a type is not being
> further expanded because it is a nested occurrence of itself?
ok.
> That is,
> given my proposed layout above, ImageInfo would turn into something like:
>
> { "name": "ImageInfo", "type": "struct",
> "data": [ { "name": "filename", "type", "str" },
> ...
> { "name": "backing-image", "type": "ImageInfo",
> "optional": true, "recursive": true } ] },
>
> > +
> > +== more description of 'DataObject' type ==
> > +
> > +We use 'DataObject' to describe dynamical data struct, it might be
>
> s/dynamical/dynamic/
>
> > +nested dictionary, list or string.
> > +
> > +'DataObject' itself is a arbitrary and nested dictionary, the
> > +dictionary has three keys ('key', 'type', 'data'), 'key' and
>
> spacing is odd.
>
> > +'data' are optional.
> > +
> > +* For describing Dictionary, we set the key to 'key', and set the
> > + value to 'type'
> > +* For describing List, we don't set 'key', just set the value to
> > + 'type'
>
> Again, if you use the idea of a discriminated union, you don't need
> quite the complexity in describing this: dictionaries are listed with
> key/type/optional tuples, lists (enums) are listed with just an array of
> strings, and the QAPI perfectly described that difference by the
> discriminator telling you 'struct' vs. 'enum'.
>
> > +* If the value of dictionary or list is non-native type, we extend
> > + the non-native type to dictionary, set it to 'data', and set the
> > + non-native type's name to 'type'.
>
> Again, I tried to set up the QAPI that describes something that uses an
> anonymous union that either gives a string (the name of a primitive or
> already-defined type) or a struct (a recursion into another layer of
> struct describing the structure of that type in line).
>
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 7b9fef1..cf03391 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3679,3 +3679,72 @@
> > '*cpuid-input-ecx': 'int',
> > 'cpuid-register': 'X86CPURegister32',
> > 'features': 'int' } }
> > +
> > +##
> > +# @DataObject
> > +#
> > +# Details of a data object, it can be nested dictionary/list
> > +#
> > +# @key: #optional Data object key
> > +#
> > +# @type: Data object type name
> > +#
> > +# @optional: #optional whether the data object is optional
>
> mention that the default is false.
>
> > +#
> > +# @data: #optional DataObject list, can be a dictionary or list type
>
> so if 'data' is present, we are describing @type in more detail; if it
> is absent, then @type is primitive.
>
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'type': 'DataObject',
> > + 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
> > +
>
> '*type' should be an enum type, not open-coded string.
Right!
> > +##
> > +# @SchemaMetaType
> > +#
> > +# Possible meta types of a schema entry
> > +#
> > +# @Command: QMP command
> > +#
> > +# @Type: defined new data type
> > +#
> > +# @Enumeration: enumeration data type
> > +#
> > +# @Union: union data type
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'enum': 'SchemaMetaType',
> > + 'data': ['Command', 'Type', 'Enumeration', 'Union'] }
>
> Do we need to start these with a capital? On the other hand, having
> them as capitals may make it easier to ensure we are outputting correct
> information.
> > +
> > +##
> > +# @SchemaEntry
> > +#
> > +# Details of schema items
> > +#
> > +# @type: Entry's type in string format
> > +#
> > +# @name: Entry name
> > +#
> > +# @data: #optional list of DataObject. This can have different meaning
> > +# depending on the 'type' value. For example, for a QMP command,
> > +# this member contains an argument listing. For an enumeration,
> > +# it contains the enum's values and so on
>
> This argues for making it a union type.
>
> > +#
> > +# @returns: #optional list of DataObject, return data after executing
> > +# QMP command
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
> > + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> > +
> > +##
> > +# @query-qmp-schema
> > +#
> > +# Query QMP schema information
> > +#
> > +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
>
> If you don't take any arguments, then the "returns an error" statement
> is impossible.
When we execute the full introspecting, we will parse the json strings
in the head file (converted from .json file). Return error if the
json string is invild in head file.
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index e075df4..e3cbe93 100644
> > --- a/qmp-commands.hx
> >
> > +/*
> > + * Use a string to record the visit path, type index of each node
> > + * will be saved to the string, indexes are split by ':'.
> > + */
> > +static char visit_path_str[1024];
>
> Is a fixed width buffer really the best solution, or does glib offer us
> something better? For example, a hash table, or even a growable string.
1024 is enough by experience, and I have a check in push_id() which
will fill string in visit_path_str.
+static void push_id(int id)
+{
+ char *end = strrchr(visit_path_str, ':');
+ char type_idx[256];
+ int num;
+
+ num = sprintf(type_idx, "%d:", id);
+
+ if (end) {
+ /* avoid overflow */
+ assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> > +
> > + for (i = 0; qmp_schema_table[i]; i++) {
> > + data = qobject_from_json(qmp_schema_table[i]);
> > + assert(data != NULL);
> > +
> > + qdict = qobject_to_qdict(data);
> > + assert(qdict != NULL);
> > +
> > + ent = qdict_first(qdict);
> > + if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
> > + && !qdict_get(qdict, "union")) {
> > + continue;
> > + }
>
> Why are we doing the work in C code, every time the command is called?
> Wouldn't it be more efficient to do the work in python code, at the time
> the qmp_schema_table C code is first generated, so that the generated
> code is already in the right format?
Right now, we parse the json string, and generate a dynamic data
struct and set it as the output of QMP.
Parse json string by qapi (python) script and generate C code ( a big
tree, it's buildup by many nested structs. We need to visit the big
static tree and generate another dynamic tree for QMP.
I think current implement is better. I will try the python solution.
> > +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> > +{
> > + SchemaEntryList *list, *last_entry, *entry;
> > + SchemaEntry *info;
> > + DataObjectList *obj_entry;
> > + DataObject *obj_info;
> > + QObject *data;
> > + QDict *qdict;
> > + int i;
> > +
> > + list = NULL;
> > + for (i = 0; qmp_schema_table[i]; i++) {
> > + data = qobject_from_json(qmp_schema_table[i]);
> > + assert(data != NULL);
> > +
> > + qdict = qobject_to_qdict(data);
> > + assert(qdict != NULL);
> > +
> > + if (qdict_get(qdict, "command")) {
> > + info = g_malloc0(sizeof(*info));
> > + info->type = SCHEMA_META_TYPE_COMMAND;
> > + info->name = strdup(qdict_get_str(qdict, "command"));
> > + } else {
> > + continue;
> > + }
> > +
>
> Don't we want to also output types (struct, union, enum) and eventually
> events, not just commands?
I think struct, union, enum are used in commands, so I didn't
repeatedly output them. If it's not repeated, I will output all of
them in next version.
> I hope we're converging, but I'm starting to worry that we won't have a
> design in place in time for the 1.6 release.
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-26 7:51 ` Amos Kong
@ 2013-07-26 11:52 ` Eric Blake
0 siblings, 0 replies; 26+ messages in thread
From: Eric Blake @ 2013-07-26 11:52 UTC (permalink / raw)
To: Amos Kong; +Cc: kwolf, aliguori, armbru, qemu-devel, lcapitulino, pbonzini
[-- Attachment #1: Type: text/plain, Size: 724 bytes --]
On 07/26/2013 01:51 AM, Amos Kong wrote:
>>> +# Query QMP schema information
>>> +#
>>> +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
>>
>> If you don't take any arguments, then the "returns an error" statement
>> is impossible.
>
> When we execute the full introspecting, we will parse the json strings
> in the head file (converted from .json file). Return error if the
> json string is invild in head file.
We should fail to compile if the conversion from .json to head file is
not correct. No working qemu binary should ever encounter an invalid
head file.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-07-19 22:05 ` Eric Blake
2013-07-26 7:51 ` Amos Kong
@ 2013-11-27 2:32 ` Amos Kong
2013-11-27 9:51 ` Kevin Wolf
[not found] ` <20131220110001.GC2890@amosk.info>
1 sibling, 2 replies; 26+ messages in thread
From: Amos Kong @ 2013-11-27 2:32 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, aliguori, armbru, qemu-devel, lcapitulino, pbonzini
[-- Attachment #1: Type: text/plain, Size: 16771 bytes --]
On Fri, Jul 19, 2013 at 04:05:16PM -0600, Eric Blake wrote:
> On 07/16/2013 04:37 AM, Amos Kong wrote:
> > Introduces new monitor command to query QMP schema information,
> > the return data is a dynamical and nested dict/list, it contains
>
> s/dynamical/dynamic/
>
> > the useful metadata to help management to check feature support,
> > QMP commands detail, etc.
> >
> > I added a document for QMP introspection support.
> > (docs/qmp-full-introspection.txt)
>
> Yay - docs make a world of difference.
>
> >
> > We need to parse all commands json definition, and generated a
>
> mixing tense ("need" present, "generated" past); for commit messages,
> present tense is generally appropriate. Thus:
>
> s/generated/generate/
>
> > dynamical tree, QMP infrastructure will convert the tree to
>
> s/dynamical/dynamic/
>
> > json string and return to QMP client.
> >
> > So here I defined a 'DataObject' type in qapi-schema.json,
> > it's used to describe the dynamical dictionary/list/string.
>
> s/dynamical/dynamic/
>
> >
> > { 'type': 'DataObject',
> > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> >
> > Not all the keys in data will be used.
> > # List: type
> > # Dict: key, type
> > # nested List: type, data
> > # nested Dict: key, type, data
>
> I wonder if we can take advantage of Kevin's work on unions to make this
> MUCH easier to use:
>
> https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html
>
> { 'enum': 'DataObjectType',
> 'data': [ 'command', 'struct', 'union', 'enum' ] }
> # extend to add 'event' later
>
> { 'type': 'DataObjectBase',
> 'data': { 'name': 'str' } }
>
> { 'union': 'DataObjectMemberType',
> 'discriminator': {},
> 'data': { 'reference': 'str',
> 'inline': 'DataObject' } }
>
> { 'type': DataObjectMember',
> 'data': { 'name': 'str', 'type': 'DataObjectMemberType',
> '*optional': 'bool', '*recursive': 'bool' } }
>
> { 'type': 'DataObjectStruct',
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectEnum',
> 'data': { 'data': [ 'str' ] } }
> { 'type': 'DataObjectUnion',
> 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> '*discriminator': 'str' } }
> { 'type': 'DataObjectCommand',
> 'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }
> { 'union': 'DataObject',
> 'base': 'DataObjectBase',
> 'discriminator': 'type',
> 'data': {
> 'struct': 'DataObjectStruct',
> 'enum': 'DataObjectEnum',
> 'union': 'DataObjectUnion',
> 'command': 'DataObjectCommand',
> 'array': {} }
In my patch, I used a _dictionary_ to describe this kind of thing
1) dict, 2) list, 3) str
The above line is used for Dictionary or List, it should be:
'array': ['DataObject']
But I touched a new error:
qapi-visit.c: In function ‘visit_type_DataObject’:
qapi-visit.c:7255:29: error: implicit declaration of function ‘visit_type_DataObjectList_fields’ [-Werror=implicit-function-declaration]
visit_type_DataObjectList_fields(m, &(*obj)->array, &err);
----
So I try to defined
{ 'type': 'DataObjectArray', 'data': ['DataObject'] }
'DataObjectArrayType' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'name',
'data': {
'array': 'DataObjectArray',
got error:
Traceback (most recent call last):
File "/home/devel/qemu/scripts/qapi-types.py", line 471, in <module>
ret += generate_struct(expr) + "\n"
File "/home/devel/qemu/scripts/qapi-types.py", line 101, in generate_struct
ret += generate_struct_fields(members)
File "/home/devel/qemu/scripts/qapi-types.py", line 67, in generate_struct_fields
for argname, argentry, optional, structured in parse_args(members):
File "/home/devel/qemu/scripts/qapi.py", line 197, in parse_args
argentry = typeinfo[member]
TypeError: list indices must be integers, not str
----
a new definition:
{ 'enum': 'DataObjectArrayType', 'data': ['Dictionary', 'List'] }
{ 'type': 'DataObjectArray', 'data': {'data': ['DataObject'], 'type':
'DataObjectArrayType' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'name',
'data': {
'array': 'DataObjectArray',
-----
In my V2, I parse the schema just according the Format attribute (dict, str, list)
Eric suggested defination is wonderful, but it's not flexible as mine ;-)
The data type, format (dict/str/list), more matadata should be considered.
It makes the parse very complex.
I have to simple it, the matadata will also provided, just make the
parse work easyer. Libvirt can still get good info as using Eric's
defination.
Thanks, Amos
> > The DataObject is described in docs/qmp-full-introspection.txt in
> > detail.
> >
> > The following content gives an example of query-tpm-types:
> >
> > ## Define example in qapi-schema.json:
> >
> > { 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> > { 'command': 'query-tpm-types', 'returns': ['TpmType'] }
>
> It might be more useful to have a (made-up) example that shows as many
> details as possible - a command that takes arguments and has a return
> type will exercise more of the code paths than a query command with only
> a return.
>
> >
> > ## Returned description:
> >
> > {
> > "name": "query-tpm-types",
> > "type": "Command",
> > "returns": [
> > {
> > "type": "TpmType",
> > "data": [
> > {
> > "type": "passthrough"
> > }
> > ]
>
> I need a way to know the difference between a TpmType returned directly
> in a dict, vs. a return containing an array of TpmType.
>
> > }
> > ]
> > },
>
> Thus, using the discriminated union I set out above, I would return
> something like:
> { "name": "TpmType", "type": "enum",
> "data": [ "passthrough" ] },
> { "name": "query-tpm-types", "type": "command",
> "data": [ ],
> "returns": { "name": "TpmType", "type": "array" } }
>
> >
> > 'TpmType' is a defined type, it will be extended in returned
> > description. [ 'passthrough' ] is a list, so 'type' and 'data'
> > will be used.
> >
> > TODO:
> > We can add events definations to qapi-schema.json by another
>
> s/definations/definitions/
>
> > patch, then event can also be queried.
> >
> > Introduce another command 'query-qga-schema' to query QGA schema
> > information, it's easy to add this support with current current
> > patch.
>
> Such a command would be part of the QGA interface, not a QMP command.
> But yes, it is needed, and the ideal patch series from you will include
> that patch as part of the series. But I don't mind waiting until we get
> the interface nailed down before you actually implement the QGA repeat
> of the interface.
>
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > docs/qmp-full-introspection.txt | 143 +++++++++++++++++++
> > qapi-schema.json | 69 +++++++++
> > qmp-commands.hx | 39 +++++
> > qmp.c | 307 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 558 insertions(+)
> > create mode 100644 docs/qmp-full-introspection.txt
> >
> > diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspection.txt
> > new file mode 100644
> > index 0000000..cc0fb80
> > --- /dev/null
> > +++ b/docs/qmp-full-introspection.txt
> > @@ -0,0 +1,143 @@
> > += full introspection support for QMP =
> > +
>
> Is it worth merging this into the existing qapi-code-gen.txt, or at
> least having the two documents refer to one another, since they deal
> with related concepts (turning the .json file into generated code)?
>
> > +== Purpose ==
> > +
> > +Add a new interface to provide QMP schema information to management,
>
> s/a new/an/ - after a year, the interface won't be new, but this doc
> will still be relevant.
>
> > +the return data is a dynamical and nested dict/list, it contains
>
> s/dynamical/dynamic/
>
> > +the useful metadata to help management to check feature support,
> > +QMP commands detail, etc.
> > +
> > +== Usage ==
> > +
> > +Execute QMP command:
> > +
> > + { "execute": "query-qmp-schema" }
> > +
> > +Returns:
> > +
> > + { "return": [
> > + {
> > + "name": "query-name",
> > + "type": "Command",
> > + "returns": [
> > + {
> > + "key": "*name",
> > + "type": "str"
> > + }
> > + ]
>
> Are we trying to use struct names where they exist for compactness, or
> trying to inline-expand everything as far as possible until we run into
> self-referential recursion?
>
> > + },
> > + ...
> > + }
> > +
> > +The whole schema information will be returned in one go, it contains
> > +commands and event. It doesn't support to be filtered by type or name.
>
> s/event/events/
> s/It/At present, it/
> s/to be filtered/filtering/
>
> > +
> > +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData)
>
> This list will get out of date quickly. I'd just word it generically:
>
> We have several self-referential types
>
> > +that uses themself in their own define data directly or indirectly,
>
> s/uses themself/use themselves/
>
> > +we will not repeatedly extend them to avoid dead loop.
>
> Would it be worth a flag in the QMP output when a type is not being
> further expanded because it is a nested occurrence of itself? That is,
> given my proposed layout above, ImageInfo would turn into something like:
>
> { "name": "ImageInfo", "type": "struct",
> "data": [ { "name": "filename", "type", "str" },
> ...
> { "name": "backing-image", "type": "ImageInfo",
> "optional": true, "recursive": true } ] },
>
> > +
> > +== more description of 'DataObject' type ==
> > +
> > +We use 'DataObject' to describe dynamical data struct, it might be
>
> s/dynamical/dynamic/
>
> > +nested dictionary, list or string.
> > +
> > +'DataObject' itself is a arbitrary and nested dictionary, the
> > +dictionary has three keys ('key', 'type', 'data'), 'key' and
>
> spacing is odd.
>
> > +'data' are optional.
> > +
> > +* For describing Dictionary, we set the key to 'key', and set the
> > + value to 'type'
> > +* For describing List, we don't set 'key', just set the value to
> > + 'type'
>
> Again, if you use the idea of a discriminated union, you don't need
> quite the complexity in describing this: dictionaries are listed with
> key/type/optional tuples, lists (enums) are listed with just an array of
> strings, and the QAPI perfectly described that difference by the
> discriminator telling you 'struct' vs. 'enum'.
>
> > +* If the value of dictionary or list is non-native type, we extend
> > + the non-native type to dictionary, set it to 'data', and set the
> > + non-native type's name to 'type'.
>
> Again, I tried to set up the QAPI that describes something that uses an
> anonymous union that either gives a string (the name of a primitive or
> already-defined type) or a struct (a recursion into another layer of
> struct describing the structure of that type in line).
>
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 7b9fef1..cf03391 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3679,3 +3679,72 @@
> > '*cpuid-input-ecx': 'int',
> > 'cpuid-register': 'X86CPURegister32',
> > 'features': 'int' } }
> > +
> > +##
> > +# @DataObject
> > +#
> > +# Details of a data object, it can be nested dictionary/list
> > +#
> > +# @key: #optional Data object key
> > +#
> > +# @type: Data object type name
> > +#
> > +# @optional: #optional whether the data object is optional
>
> mention that the default is false.
>
> > +#
> > +# @data: #optional DataObject list, can be a dictionary or list type
>
> so if 'data' is present, we are describing @type in more detail; if it
> is absent, then @type is primitive.
>
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'type': 'DataObject',
> > + 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data': ['DataObject'] } }
> > +
>
> '*type' should be an enum type, not open-coded string.
>
>
> > +##
> > +# @SchemaMetaType
> > +#
> > +# Possible meta types of a schema entry
> > +#
> > +# @Command: QMP command
> > +#
> > +# @Type: defined new data type
> > +#
> > +# @Enumeration: enumeration data type
> > +#
> > +# @Union: union data type
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'enum': 'SchemaMetaType',
> > + 'data': ['Command', 'Type', 'Enumeration', 'Union'] }
>
> Do we need to start these with a capital? On the other hand, having
> them as capitals may make it easier to ensure we are outputting correct
> information.
> > +
> > +##
> > +# @SchemaEntry
> > +#
> > +# Details of schema items
> > +#
> > +# @type: Entry's type in string format
> > +#
> > +# @name: Entry name
> > +#
> > +# @data: #optional list of DataObject. This can have different meaning
> > +# depending on the 'type' value. For example, for a QMP command,
> > +# this member contains an argument listing. For an enumeration,
> > +# it contains the enum's values and so on
>
> This argues for making it a union type.
>
> > +#
> > +# @returns: #optional list of DataObject, return data after executing
> > +# QMP command
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType',
> > + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> > +
> > +##
> > +# @query-qmp-schema
> > +#
> > +# Query QMP schema information
> > +#
> > +# Returns: list of @SchemaEntry. Returns an error if json string is invalid.
>
> If you don't take any arguments, then the "returns an error" statement
> is impossible.
>
> > +#
> > +# Since: 1.6
> > +##
> > +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index e075df4..e3cbe93 100644
> > --- a/qmp-commands.hx
> >
> > +/*
> > + * Use a string to record the visit path, type index of each node
> > + * will be saved to the string, indexes are split by ':'.
> > + */
> > +static char visit_path_str[1024];
>
> Is a fixed width buffer really the best solution, or does glib offer us
> something better? For example, a hash table, or even a growable string.
>
> > +
> > + for (i = 0; qmp_schema_table[i]; i++) {
> > + data = qobject_from_json(qmp_schema_table[i]);
> > + assert(data != NULL);
> > +
> > + qdict = qobject_to_qdict(data);
> > + assert(qdict != NULL);
> > +
> > + ent = qdict_first(qdict);
> > + if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type")
> > + && !qdict_get(qdict, "union")) {
> > + continue;
> > + }
>
> Why are we doing the work in C code, every time the command is called?
> Wouldn't it be more efficient to do the work in python code, at the time
> the qmp_schema_table C code is first generated, so that the generated
> code is already in the right format?
>
> > +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> > +{
> > + SchemaEntryList *list, *last_entry, *entry;
> > + SchemaEntry *info;
> > + DataObjectList *obj_entry;
> > + DataObject *obj_info;
> > + QObject *data;
> > + QDict *qdict;
> > + int i;
> > +
> > + list = NULL;
> > + for (i = 0; qmp_schema_table[i]; i++) {
> > + data = qobject_from_json(qmp_schema_table[i]);
> > + assert(data != NULL);
> > +
> > + qdict = qobject_to_qdict(data);
> > + assert(qdict != NULL);
> > +
> > + if (qdict_get(qdict, "command")) {
> > + info = g_malloc0(sizeof(*info));
> > + info->type = SCHEMA_META_TYPE_COMMAND;
> > + info->name = strdup(qdict_get_str(qdict, "command"));
> > + } else {
> > + continue;
> > + }
> > +
>
> Don't we want to also output types (struct, union, enum) and eventually
> events, not just commands?
>
> I hope we're converging, but I'm starting to worry that we won't have a
> design in place in time for the 1.6 release.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-11-27 2:32 ` Amos Kong
@ 2013-11-27 9:51 ` Kevin Wolf
[not found] ` <20131220110001.GC2890@amosk.info>
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2013-11-27 9:51 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, armbru, qemu-devel, lcapitulino, pbonzini
[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]
Am 27.11.2013 um 03:32 hat Amos Kong geschrieben:
> On Fri, Jul 19, 2013 at 04:05:16PM -0600, Eric Blake wrote:
> > On 07/16/2013 04:37 AM, Amos Kong wrote:
> > > { 'type': 'DataObject',
> > > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } }
> > >
> > > Not all the keys in data will be used.
> > > # List: type
> > > # Dict: key, type
> > > # nested List: type, data
> > > # nested Dict: key, type, data
> >
> > I wonder if we can take advantage of Kevin's work on unions to make this
> > MUCH easier to use:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html
> >
> > { 'enum': 'DataObjectType',
> > 'data': [ 'command', 'struct', 'union', 'enum' ] }
> > # extend to add 'event' later
> >
> > { 'type': 'DataObjectBase',
> > 'data': { 'name': 'str' } }
> >
> > { 'union': 'DataObjectMemberType',
> > 'discriminator': {},
> > 'data': { 'reference': 'str',
> > 'inline': 'DataObject' } }
> >
> > { 'type': DataObjectMember',
> > 'data': { 'name': 'str', 'type': 'DataObjectMemberType',
> > '*optional': 'bool', '*recursive': 'bool' } }
> >
> > { 'type': 'DataObjectStruct',
> > 'data': { 'data': [ 'DataObjectMember' ] } }
> > { 'type': 'DataObjectEnum',
> > 'data': { 'data': [ 'str' ] } }
> > { 'type': 'DataObjectUnion',
> > 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> > '*discriminator': 'str' } }
> > { 'type': 'DataObjectCommand',
> > 'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } }
>
>
> > { 'union': 'DataObject',
> > 'base': 'DataObjectBase',
> > 'discriminator': 'type',
> > 'data': {
> > 'struct': 'DataObjectStruct',
> > 'enum': 'DataObjectEnum',
> > 'union': 'DataObjectUnion',
> > 'command': 'DataObjectCommand',
>
> > 'array': {} }
>
>
> In my patch, I used a _dictionary_ to describe this kind of thing
> 1) dict, 2) list, 3) str
>
> The above line is used for Dictionary or List, it should be:
> 'array': ['DataObject']
>
> But I touched a new error:
> qapi-visit.c: In function ‘visit_type_DataObject’:
> qapi-visit.c:7255:29: error: implicit declaration of function ‘visit_type_DataObjectList_fields’ [-Werror=implicit-function-declaration]
> visit_type_DataObjectList_fields(m, &(*obj)->array, &err);
>
> ----
> So I try to defined
>
> { 'type': 'DataObjectArray', 'data': ['DataObject'] }
> 'DataObjectArrayType' } }
>
> { 'union': 'DataObject',
> 'base': 'DataObjectBase',
> 'discriminator': 'name',
> 'data': {
> 'array': 'DataObjectArray',
>
> got error:
> Traceback (most recent call last):
> File "/home/devel/qemu/scripts/qapi-types.py", line 471, in <module>
> ret += generate_struct(expr) + "\n"
> File "/home/devel/qemu/scripts/qapi-types.py", line 101, in generate_struct
> ret += generate_struct_fields(members)
> File "/home/devel/qemu/scripts/qapi-types.py", line 67, in generate_struct_fields
> for argname, argentry, optional, structured in parse_args(members):
> File "/home/devel/qemu/scripts/qapi.py", line 197, in parse_args
> argentry = typeinfo[member]
> TypeError: list indices must be integers, not str
I've never had a need for unions inside an array so far, but that
doesn't make them less useful. We'll need them in more places than just
here.
So instead of working around the current limitations, the right thing to
do is to modify the QAPI generator to allow this kind of definitions.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
[not found] ` <20131220110001.GC2890@amosk.info>
@ 2013-12-20 11:57 ` Amos Kong
2013-12-20 18:03 ` Paolo Bonzini
2013-12-23 6:32 ` Wenchao Xia
0 siblings, 2 replies; 26+ messages in thread
From: Amos Kong @ 2013-12-20 11:57 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, mdroth, qemu-devel, lcapitulino, qiaonuohan, pbonzini, xiawenc
[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]
(resend without big attachment)
Hello Eric, other
We had "command, enumeration, type, unionobj" in Eric suggested DataObject
union, it's helpful for us to provide meaningful metadata in the output.
but there still exists some problem.
We should describe some arbitrary data struct, I would like to call it "undefined struct"
eg 1:
{ 'type': 'VersionInfo',
'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
'package': 'str'} }
it's same as:
{ 'type': 'newtype',
'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
'data': { 'qemu': 'newtype', 'package': 'str'} }
The difference between original 'DataObjectType' and 'DataObjectUndefinedStruct'
is that we don't have 'name' for the DataObject union for undefined struct.
so I set the 'name' item in DataObjectBase to be optional.
eg 2:
{ 'command': 'human-monitor-command',
'data': {'command-line': 'str', '*cpu-index': 'int'},
'returns': 'str' }
... 'returns': ['RxFilterInfo'] }
... 'returns': 'ChardevReturn' }
We returns str (native type), list or extended dict here. Sometimes we
returns a defined type, but it doesn't need to be extended. And we need
to describe this kind of data (type str, dict or list) by "DataObject"
in schema definition of DataObject** type.
So I added a "'reference-type': 'String'" in DataObject union.
list, dict will still be described by "DataObjectType"
You can find the draft patches here:
https://github.com/kongove/qemu/commits/qmp-introspection
I will post the V3 when I finish the cleanup.
Thanks, Amos
Command output
==============
https://raw.github.com/kongove/misc/master/txt/qmp-introspection.output.txt (1.6M)
Latest schema definition
========================
{ 'type': 'DataObjectBase',
'data': { '*name': 'str', 'type': 'str' } }
{ 'union': 'DataObjectMemberType',
'discriminator': {},
'data': { 'reference': 'str',
'undefined': 'DataObject',
'extend': 'DataObject' } }
{ 'type': 'DataObjectMember',
'data': { 'type': 'DataObjectMemberType', '*name': 'str',
'*optional': 'bool', '*recursive': 'bool' } }
{ 'type': 'DataObjectCommand',
'data': { '*data': [ 'DataObjectMember' ],
'*returns': 'DataObject',
'*gen': 'bool' } }
{ 'type': 'DataObjectEnumeration',
'data': { 'data': [ 'str' ] } }
{ 'type': 'DataObjectType',
'data': { 'data': [ 'DataObjectMember' ] } }
{ 'type': 'DataObjectUndefinedStruct',
'data': { 'data': [ 'DataObjectMember' ] } }
{ 'type': 'DataObjectUnion',
'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
'*discriminator': 'str' } }
{ 'union': 'DataObject',
'base': 'DataObjectBase',
'discriminator': 'type',
'data': {
'command': 'DataObjectCommand',
'enumeration': 'DataObjectEnumeration',
'type': 'DataObjectType',
'undefined-struct': 'DataObjectUndefinedStruct',
'reference-type': 'String',
'unionobj': 'DataObjectUnion' } }
{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-12-20 11:57 ` Amos Kong
@ 2013-12-20 18:03 ` Paolo Bonzini
2013-12-23 8:11 ` Amos Kong
2013-12-23 6:32 ` Wenchao Xia
1 sibling, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2013-12-20 18:03 UTC (permalink / raw)
To: Amos Kong; +Cc: kwolf, mdroth, qemu-devel, lcapitulino, qiaonuohan, xiawenc
Il 20/12/2013 12:57, Amos Kong ha scritto:
> { 'type': 'DataObjectBase',
> 'data': { '*name': 'str', 'type': 'str' } }
> { 'union': 'DataObjectMemberType',
> 'discriminator': {},
> 'data': { 'reference': 'str',
> 'undefined': 'DataObject',
> 'extend': 'DataObject' } }
What is the purpose of "undefined"? I don't see any occurrence of any
of "undefined" or "extend" in the sample.
> { 'type': 'DataObjectMember',
> 'data': { 'type': 'DataObjectMemberType', '*name': 'str',
> '*optional': 'bool', '*recursive': 'bool' } }
> { 'type': 'DataObjectCommand',
> 'data': { '*data': [ 'DataObjectMember' ],
> '*returns': 'DataObject',
> '*gen': 'bool' } }
> { 'type': 'DataObjectEnumeration',
> 'data': { 'data': [ 'str' ] } }
> { 'type': 'DataObjectType',
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectUndefinedStruct',
Perhaps Unnamed or Anonymous?
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectUnion',
> 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> '*discriminator': 'str' } }
> { 'union': 'DataObject',
> 'base': 'DataObjectBase',
> 'discriminator': 'type',
> 'data': {
> 'command': 'DataObjectCommand',
> 'enumeration': 'DataObjectEnumeration',
> 'type': 'DataObjectType',
> 'undefined-struct': 'DataObjectUndefinedStruct',
> 'reference-type': 'String',
> 'unionobj': 'DataObjectUnion' } }
> { 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
I think forcing expansion of everything that isn't unnamed/anonymous
makes the schema much larger and unwieldy. Otherwise looks great!
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-12-20 11:57 ` Amos Kong
2013-12-20 18:03 ` Paolo Bonzini
@ 2013-12-23 6:32 ` Wenchao Xia
2013-12-23 7:15 ` Amos Kong
1 sibling, 1 reply; 26+ messages in thread
From: Wenchao Xia @ 2013-12-23 6:32 UTC (permalink / raw)
To: Amos Kong, Eric Blake
Cc: kwolf, mdroth, qemu-devel, qiaonuohan, pbonzini, lcapitulino
Hi, Amos
> (resend without big attachment)
>
> Hello Eric, other
>
> We had "command, enumeration, type, unionobj" in Eric suggested DataObject
> union, it's helpful for us to provide meaningful metadata in the output.
> but there still exists some problem.
>
> We should describe some arbitrary data struct, I would like to call it "undefined struct"
>
If user have defined an arbitrary or embbed data struct, I think it
is better leave as it is, instead of generate a new struct for it.
Since qapi-visit.c and qapi-types.c doesn't have a "undefined" struct
for it now, it is a bit risk to do it only in QMP introspection. Maybe
leave it now, and support it when we found it is really useful?(and add
it in qapi-visit.c and qapi-types.c correspondly)
> eg 1:
> { 'type': 'VersionInfo',
> 'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> 'package': 'str'} }
> it's same as:
> { 'type': 'newtype',
> 'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
> { 'type': 'VersionInfo',
> 'data': { 'qemu': 'newtype', 'package': 'str'} }
>
> The difference between original 'DataObjectType' and 'DataObjectUndefinedStruct'
> is that we don't have 'name' for the DataObject union for undefined struct.
> so I set the 'name' item in DataObjectBase to be optional.
>
> eg 2:
> { 'command': 'human-monitor-command',
> 'data': {'command-line': 'str', '*cpu-index': 'int'},
> 'returns': 'str' }
> ... 'returns': ['RxFilterInfo'] }
> ... 'returns': 'ChardevReturn' }
>
> We returns str (native type), list or extended dict here. Sometimes we
> returns a defined type, but it doesn't need to be extended. And we need
> to describe this kind of data (type str, dict or list) by "DataObject"
> in schema definition of DataObject** type.
>
> So I added a "'reference-type': 'String'" in DataObject union.
> list, dict will still be described by "DataObjectType"
>
I guess you want to tip what type may be returned? It seems a bit too
agressive, since the qapi-schema.json didn't tip that those types may
be returned.
>
> You can find the draft patches here:
> https://github.com/kongove/qemu/commits/qmp-introspection
> I will post the V3 when I finish the cleanup.
>
> Thanks, Amos
>
>
> Command output
> ==============
> https://raw.github.com/kongove/misc/master/txt/qmp-introspection.output.txt (1.6M)
>
> Latest schema definition
> ========================
> { 'type': 'DataObjectBase',
> 'data': { '*name': 'str', 'type': 'str' } }
> { 'union': 'DataObjectMemberType',
> 'discriminator': {},
> 'data': { 'reference': 'str',
> 'undefined': 'DataObject',
> 'extend': 'DataObject' } }
> { 'type': 'DataObjectMember',
> 'data': { 'type': 'DataObjectMemberType', '*name': 'str',
> '*optional': 'bool', '*recursive': 'bool' } }
> { 'type': 'DataObjectCommand',
> 'data': { '*data': [ 'DataObjectMember' ],
> '*returns': 'DataObject',
> '*gen': 'bool' } }
> { 'type': 'DataObjectEnumeration',
> 'data': { 'data': [ 'str' ] } }
> { 'type': 'DataObjectType',
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectUndefinedStruct',
> 'data': { 'data': [ 'DataObjectMember' ] } }
> { 'type': 'DataObjectUnion',
> 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> '*discriminator': 'str' } }
> { 'union': 'DataObject',
> 'base': 'DataObjectBase',
> 'discriminator': 'type',
> 'data': {
> 'command': 'DataObjectCommand',
> 'enumeration': 'DataObjectEnumeration',
> 'type': 'DataObjectType',
> 'undefined-struct': 'DataObjectUndefinedStruct',
> 'reference-type': 'String',
> 'unionobj': 'DataObjectUnion' } }
> { 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-12-23 6:32 ` Wenchao Xia
@ 2013-12-23 7:15 ` Amos Kong
0 siblings, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-12-23 7:15 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino, qiaonuohan, pbonzini
On Mon, Dec 23, 2013 at 02:32:46PM +0800, Wenchao Xia wrote:
> Hi, Amos
>
> >(resend without big attachment)
> >
> >Hello Eric, other
> >
> >We had "command, enumeration, type, unionobj" in Eric suggested DataObject
> >union, it's helpful for us to provide meaningful metadata in the output.
> >but there still exists some problem.
> >
> >We should describe some arbitrary data struct, I would like to call it "undefined struct"
> >
> If user have defined an arbitrary or embbed data struct, I think it
> is better leave as it is, instead of generate a new struct for it.
I don't really generate a new struct for it, just try the arbitrary
data as an abstract 'undefined' struct.
In the output, it's "leave as it is".
> Since qapi-visit.c and qapi-types.c doesn't have a "undefined" struct
> for it now, it is a bit risk to do it only in QMP introspection. Maybe
> leave it now, and support it when we found it is really useful?(and add
> it in qapi-visit.c and qapi-types.c correspondly)
>
> >eg 1:
> > { 'type': 'VersionInfo',
> > 'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> > 'package': 'str'} }
> > it's same as:
> > { 'type': 'newtype',
> > 'data': {'major': 'int', 'minor': 'int', 'micro': 'int'} }
> > { 'type': 'VersionInfo',
> > 'data': { 'qemu': 'newtype', 'package': 'str'} }
> >
> > The difference between original 'DataObjectType' and 'DataObjectUndefinedStruct'
> > is that we don't have 'name' for the DataObject union for undefined struct.
> > so I set the 'name' item in DataObjectBase to be optional.
> >
> >eg 2:
> > { 'command': 'human-monitor-command',
> > 'data': {'command-line': 'str', '*cpu-index': 'int'},
> > 'returns': 'str' }
> > ... 'returns': ['RxFilterInfo'] }
> > ... 'returns': 'ChardevReturn' }
> >
> > We returns str (native type), list or extended dict here. Sometimes we
> > returns a defined type, but it doesn't need to be extended. And we need
> > to describe this kind of data (type str, dict or list) by "DataObject"
> > in schema definition of DataObject** type.
> >
> > So I added a "'reference-type': 'String'" in DataObject union.
> > list, dict will still be described by "DataObjectType"
> >
> I guess you want to tip what type may be returned? It seems a bit too
> agressive, since the qapi-schema.json didn't tip that those types may
> be returned.
In command schema, the value of 'returns' tips the model of return data.
It can be string(defined type/union/enum/etc) or undefined list/dict.
> >You can find the draft patches here:
> > https://github.com/kongove/qemu/commits/qmp-introspection
> >I will post the V3 when I finish the cleanup.
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP
2013-12-20 18:03 ` Paolo Bonzini
@ 2013-12-23 8:11 ` Amos Kong
0 siblings, 0 replies; 26+ messages in thread
From: Amos Kong @ 2013-12-23 8:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, mdroth, qemu-devel, lcapitulino, qiaonuohan, xiawenc
On Fri, Dec 20, 2013 at 07:03:57PM +0100, Paolo Bonzini wrote:
> Il 20/12/2013 12:57, Amos Kong ha scritto:
> > { 'type': 'DataObjectBase',
> > 'data': { '*name': 'str', 'type': 'str' } }
> > { 'union': 'DataObjectMemberType',
> > 'discriminator': {},
> > 'data': { 'reference': 'str',
> > 'undefined': 'DataObject',
> > 'extend': 'DataObject' } }
>
> What is the purpose of "undefined"? I don't see any occurrence of any
> of "undefined" or "extend" in the sample.
| { 'type': 'VersionInfo',
| 'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| {
| "name": "VersionInfo",
| "type": "type",
| "data": [
|~ {
|~ "name": "qemu",
|~ "optional": false,
|~ "type": {
|~ "type": "undefined-struct",
|~ "data": [
|~ {
|~ "name": "micro",
|~ "optional": false,
|~ "recursive": false,
|~ "type": "int"
|~ },
|~ {
|~ "name": "minor",
|~ "optional": false,
|~ "recursive": false,
|~ "type": "int"
|~ },
|~ {
|~ "name": "major",
|~ "optional": false,
|~ "recursive": false,
|~ "type": "int"
|~ }
|~ ]
|~ }
|~ },
> > { 'type': 'DataObjectMember',
> > 'data': { 'type': 'DataObjectMemberType', '*name': 'str',
> > '*optional': 'bool', '*recursive': 'bool' } }
> > { 'type': 'DataObjectCommand',
> > 'data': { '*data': [ 'DataObjectMember' ],
> > '*returns': 'DataObject',
> > '*gen': 'bool' } }
> > { 'type': 'DataObjectEnumeration',
> > 'data': { 'data': [ 'str' ] } }
> > { 'type': 'DataObjectType',
> > 'data': { 'data': [ 'DataObjectMember' ] } }
> > { 'type': 'DataObjectUndefinedStruct',
>
> Perhaps Unnamed or Anonymous?
Anonymous is good.
> > 'data': { 'data': [ 'DataObjectMember' ] } }
> > { 'type': 'DataObjectUnion',
> > 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> > '*discriminator': 'str' } }
> > { 'union': 'DataObject',
> > 'base': 'DataObjectBase',
> > 'discriminator': 'type',
> > 'data': {
> > 'command': 'DataObjectCommand',
> > 'enumeration': 'DataObjectEnumeration',
> > 'type': 'DataObjectType',
> > 'undefined-struct': 'DataObjectUndefinedStruct',
> > 'reference-type': 'String',
> > 'unionobj': 'DataObjectUnion' } }
> > { 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
>
> I think forcing expansion of everything that isn't unnamed/anonymous
> makes the schema much larger and unwieldy. Otherwise looks great!
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.
> Paolo
--
Amos.
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2013-12-23 8:11 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-16 10:37 [Qemu-devel] [PATCH v2 0/2] QMP full introspection Amos Kong
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 1/2] qapi: change qapi to convert schema json Amos Kong
2013-07-17 20:09 ` Luiz Capitulino
2013-07-26 3:39 ` Amos Kong
2013-07-19 12:27 ` Eric Blake
2013-07-26 6:53 ` Amos Kong
2013-07-16 10:37 ` [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP Amos Kong
2013-07-16 10:48 ` Paolo Bonzini
2013-07-16 11:04 ` Amos Kong
2013-07-16 11:08 ` Paolo Bonzini
2013-07-16 12:04 ` Amos Kong
2013-07-16 12:18 ` Paolo Bonzini
2013-07-26 7:03 ` Amos Kong
2013-07-17 20:36 ` Luiz Capitulino
2013-07-19 20:26 ` Eric Blake
2013-07-26 7:21 ` Amos Kong
2013-07-19 22:05 ` Eric Blake
2013-07-26 7:51 ` Amos Kong
2013-07-26 11:52 ` Eric Blake
2013-11-27 2:32 ` Amos Kong
2013-11-27 9:51 ` Kevin Wolf
[not found] ` <20131220110001.GC2890@amosk.info>
2013-12-20 11:57 ` Amos Kong
2013-12-20 18:03 ` Paolo Bonzini
2013-12-23 8:11 ` Amos Kong
2013-12-23 6:32 ` Wenchao Xia
2013-12-23 7:15 ` Amos Kong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.