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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 19 Jun 2013 20:24:37 +0800
Amos Kong <akong@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 <akong@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 <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 *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: