On Wed, 19 Jun 2013 20:24:37 +0800
Amos Kong <ak...@redhat.com> wrote:
> Introduces new monitor command to query QMP schema information,
> the return data is a nested dict/list, it contains the useful
> metadata.
Thanks for the good work, Amos!
When testing this though I actually get qemu-ga's schema, not
qmp's. Did you test this with qemu-ga build enabled?
This bug shows that we need to handle qemu-ga properly, which
means having query-guest-agent-schema for qemu-ga.
It's also a good idea to start the commit log with some json
examples btw.
More comments below.
> we can add events definations to qapi-schema.json, then it can
> also be queried.
>
> Signed-off-by: Amos Kong <ak...@redhat.com>
> ---
> Makefile | 4 +-
> qapi-schema.json | 68 +++++++++++++++++++
> qmp-commands.hx | 39 +++++++++++
> qmp.c | 170
> +++++++++++++++++++++++++++++++++++++++++++++++
> scripts/qapi-commands.py | 2 +-
> scripts/qapi-types.py | 34 +++++++++-
> scripts/qapi-visit.py | 2 +-
> scripts/qapi.py | 7 +-
> 8 files changed, 320 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 3cfa7d0..42713ef 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
> @@ -213,7 +213,7 @@ qga/qapi-generated/qga-qmp-commands.h
> qga/qapi-generated/qga-qmp-marshal.c :\
> $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> $(qapi-py)
> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, " GEN $@")
>
> -qapi-types.c qapi-types.h :\
> +qapi-types.c qapi-types.h 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 "." -b < $<, " GEN $@")
> qapi-visit.c qapi-visit.h :\
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 6cc07c2..43abe57 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3608,3 +3608,71 @@
> '*cpuid-input-ecx': 'int',
> 'cpuid-register': 'X86CPURegister32',
> 'features': 'int' } }
> +
> +##
> +# @DataObject
> +#
> +# Details of a data object, it can be nested dictionary/list
> +#
> +# @name: #optional the string key of dictionary
Data object name if it has one?
> +#
> +# @type: the string value of dictionary or list
data type name?
> +#
> +# @data: #optional a list of @DataObject, dictionary's value is nested
> +# dictionary/list
#optional DataObject list, can be a dictionary or list type?
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'DataObject',
> + 'data': { '*name': 'str', '*type': 'str', '*data': ['DataObject'] } }
> +
> +##
> +# @SchemaMetatype
As we're doing CamelCase, this should be SchemaMetaType. Or maybe just
SchemaType?
> +#
> +# Possible meta types of a schema entry
> +#
> +# @Command: QMP monitor command to control guest
"QMP command" is good enough.
> +#
> +# @Type: defined new data type
> +#
> +# @Enumeration: enumeration data type
> +#
> +# @Union: union data type
> +#
> +# @Event: QMP event to notify QMP clients
I'm not sure we should have events listed here as they are not
supported yet.
> +#
> +# Since: 1.6
> +##
> +{ 'enum': 'SchemaMetatype',
> + 'data': ['Command', 'Type', 'Enumeration', 'Union', 'Event'] }
> +
> +##
> +# @SchemaData
Sorry for the bikeshed, but SchemaEntry maybe?
> +#
> +# Details of schema items
> +#
> +# @type: dict's value, list's value
Entry's type in string format.
> +#
> +# @name: dict's key
Entry name.
> +#
> +# @data: #optional list of @DataObject, arguments data of executing
> +# QMP command
"#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
I don't parse what's after the coma.
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'SchemaData', 'data': { 'type': 'SchemaMetatype',
> + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# Returns: list of @SchemaData. Returns an error if json string is invalid.
I don't think you should return errors, see below.
> +#
> +# Since: 1.6
> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['SchemaData'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 8cea5e5..667d9ab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2997,3 +2997,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": "NameInfo",
> + "type": "Type",
> + "data": [
> + {
> + "name": "*name",
> + "type": "str"
> + }
Should we have an 'optional' bool field instead of having the *
in name?
> + ]
> + }
> + ]
> + }
> +
> +EQMP
> diff --git a/qmp.c b/qmp.c
> index 4c149b3..3a7c403 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,174 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error
> **errp)
> return arch_query_cpu_definitions(errp);
> }
>
> +static DataObjectList *visit_qobj_dict(QObject *data);
I don't think this is needed.
> +
> +static DataObjectList *visit_qobj_list(QObject *data)
> +{
> + DataObjectList *obj_list = NULL, *obj_last_entry, *obj_entry;
> + DataObject *obj_info;
> + const QListEntry *ent;
> + QList *qlist;
> +
> + qlist = qobject_to_qlist(data);
> + if (!qlist) {
> + return 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 (qobject_to_qstring(ent->value)) {
> + obj_info->has_type = true;
> + obj_info->type = g_strdup_printf("%s",
> + qstring_get_str(qobject_to_qstring(ent->value)));
> + continue;
> + }
> +
> + obj_info->has_data = 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);
> + }
> + }
> +
> + return obj_list;
> +}
> +
> +static DataObjectList *visit_qobj_dict(QObject *data)
> +{
> + DataObjectList *obj_list = NULL, *obj_last_entry, *obj_entry;
> + DataObject *obj_info;
> + const QDictEntry *ent;
> + QDict *qdict;
> +
> + qdict = qobject_to_qdict(data);
> + if (!qdict) {
> + return 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;
> +
> + obj_info->name = ent->key;
> + obj_info->has_name = true;
> + if (qobject_to_qstring(ent->value)) {
> + obj_info->has_type = true;
> + obj_info->type = g_strdup_printf("%s",
> + qstring_get_str(qobject_to_qstring(ent->value)));
> + continue;
> + }
> +
> + obj_info->has_data = 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);
> + }
> + }
> +
> + return obj_list;
> +}
> +
> +SchemaDataList *qmp_query_qmp_schema(Error **errp)
> +{
> + SchemaDataList *list = NULL, *last_entry, *entry;
> + SchemaData *info;
> + int i;
> +
> + DataObjectList *obj_entry;
> + DataObject *obj_info;
> +
> + QObject *data;
> + QDict *qdict;
> + const QDictEntry *ent;
No need for spaces between declarations.
> +
> + for (i = 0; qmp_schema_table[i]; i++) {
> + data = qobject_from_json(qmp_schema_table[i]);
> + if (!data) {
> + error_setg(errp, "Can't convert json string to valid qobject");
> + return NULL;
> + }
This should only fail if we have a bug in the qapi scripts, right? In this
case we should abort instead.
> +
> + qdict = qobject_to_qdict(data);
> + if (!qdict) {
> + error_setg(errp, "Can't convert qobject to valid qdict");
> + return NULL;
> + }
Same here.
> +
> + ent = qdict_first(qdict);
I don't think this is fully correct. This works probably because the
command/type/enum/union key is the first in the dict, but we shouldn't
count on that.
I think you should use qdict_get() here, like:
> + info = g_malloc0(sizeof(*info));
> +
> + if (!strcmp(ent->key, "command")) {
if (qdict_get(qdict, "command")) {
...
}
This is safer I think.
> + info->type = SCHEMA_METATYPE_COMMAND;
> + } else if (!strcmp(ent->key, "type")) {
> + info->type = SCHEMA_METATYPE_TYPE;
> + } else if (!strcmp(ent->key, "enum")) {
> + info->type = SCHEMA_METATYPE_ENUMERATION;
> + } else if (!strcmp(ent->key, "union")) {
> + info->type = SCHEMA_METATYPE_UNION;
> + } else if (!strcmp(ent->key, "event")) {
> + info->type = SCHEMA_METATYPE_EVENT;
> + }
> +
> + info->name = g_strdup_printf("%s",
> + qstring_get_str(qobject_to_qstring(ent->value)));
> +
> + data = qdict_get(qdict, "data");
> + if (data) {
> + if (qobject_to_qstring(data)) {
> + obj_info = g_malloc0(sizeof(*obj_info));
> + obj_entry = g_malloc0(sizeof(*obj_entry));
> + obj_entry->value = obj_info;
> + obj_info->name = g_strdup_printf("%s",
> + qstring_get_str(qobject_to_qstring(data)));
Please, add a new method to qstring, like qstring_copy_str() and
use that instead.
> + obj_info->has_type = true;
> + obj_info->type = g_strdup_printf("%s",
> + qstring_get_str(qobject_to_qstring(data)));
> + } else if (data->type->code == QTYPE_QLIST) {
> + info->has_data = true;
> + info->data = visit_qobj_list(data);
> + } else if (data->type->code == QTYPE_QDICT) {
> + info->has_data = true;
> + info->data = visit_qobj_dict(data);
> + }
> + }
> +
> + 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)
> 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..eb59a5a 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:
> @@ -303,7 +304,38 @@ 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 <ak...@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 *qmp_schema_table[] = {
I think you want const char *const qmp_schema_table[].
Btw, I find your approach interesting but I'm wondering if it's going to
be a good thing to keep all the schema in memory. Do you have an idea
on its size?
> +"""
> +
> +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("qmp-schema.h", "w")
> +f.write(schema_table)
> +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 02ad668..fe7648c 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -80,6 +80,7 @@ def evaluate(string):
>
> def parse_schema(fp):
> exprs = []
> + raw_exprs = []
> expr = ''
> expr_eval = None
>
> @@ -91,6 +92,8 @@ def parse_schema(fp):
> expr += line
> elif expr:
> expr_eval = evaluate(expr)
> + raw_exprs.append(expr)
> +
> if expr_eval.has_key('enum'):
> add_enum(expr_eval['enum'])
> elif expr_eval.has_key('union'):
> @@ -102,13 +105,15 @@ def parse_schema(fp):
>
> if expr:
> expr_eval = evaluate(expr)
> + raw_exprs.append(expr)
> +
> if expr_eval.has_key('enum'):
> add_enum(expr_eval['enum'])
> elif expr_eval.has_key('union'):
> add_enum('%sKind' % expr_eval['union'])
> exprs.append(expr_eval)
>
> - return exprs
> + return exprs, raw_exprs
>
> def parse_args(typeinfo):
> for member in typeinfo:
