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

[Luiz Capitulino]

On Tue, 16 Jul 2013 18:37:42 +0800
Amos Kong <address@hidden> 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 <address@hidden>
> ---
>  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)