Re: [Qemu-devel] [PATCH] full introspection support for QMP

[Amos Kong]

On Thu, Jun 20, 2013 at 11:20:21PM -0400, Luiz Capitulino wrote:
> On Wed, 19 Jun 2013 20:24:37 +0800
> Amos Kong <address@hidden> 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 <address@hidden>
> > ---
> >  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 <address@hidden>
> > + *
> > + * 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.