On Thu, Jun 20, 2013 at 11:20:21PM -0400, Luiz Capitulino wrote:
> 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 your comments.
> 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?
Currently qmp-schema.h will be generated two times. QMP's schema
will cover qga's schema. I will add an option '-i' / '--instrospec'
for qapi-types.py to set the name of generated file.
> This bug shows that we need to handle qemu-ga properly, which
> means having query-guest-agent-schema for qemu-ga.
I will generate two schema tables for qmp & qga
qmp-schema.h qga-schema.h
> It's also a good idea to start the commit log with some json
> examples btw.
OK, I will give some example in commitlog, and add a doc to
describe dynamical DataObject.
> 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.
Will remove it first.
> > +#
> > +# 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.
#optional list of DataObject. This is the return date of executing a QMP
command.
> > +#
> > +# 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?
OK.
> > + ]
> > + }
> > + ]
> > + }
> > +
> > +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.
visit_qobj_dict() and visit_qobj_list() call each other.
the declaration is necessary.
> > +
> > +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.
Yes, qapi script didn't provide valid schema table.
Ok, will replace the error by abort.
> > +
> > + 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.
OK.
> > + 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.
OK.
> > + 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?
The size of qmp_schema_table[] is 1528 bytes.
method 2rd:
save the raw json strings to qmp-schema.txt, allocate dynamical
qmp_schema_table[] in qmp_query_qmp_schema(), and restore the
file content to the table, free it at the end of qmp_query_qmp_schema()
qmp-schema.txt:
# Schema json strings converted from qapi-schema.json
# comments ....
# comments ....
{ 'type': 'NameInfo', 'data': {'*name': 'str'} }
{ 'command': 'query-name', 'returns': 'NameInfo' }
....
> > +"""
> > +
> > +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)
--
Amos.
