diff mbox

[v4,4/5] qmp: full introspection support for QMP

Message ID 1390488396-16538-5-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong Jan. 23, 2014, 2:46 p.m. UTC
This patch introduces a new monitor command to query QMP schema
information, the return data is a range of schema structs, which
contains the useful metadata to help management to check supported
features, QMP commands detail, etc.

We use qapi-introspect.py to parse all json definition in
qapi-schema.json, and generate a range of dictionaries with metadata.
The query command will visit the dictionaries and fill the data
to allocated struct tree. Then QMP infrastructure will convert
the tree to json string and return to QMP client.

TODO:
Wenchao Xia is working to convert QMP events to qapi-schema.json,
then event can also be queried by this interface.

I will introduce another command 'query-qga-schema' to query QGA
schema information, it's easy to add this support based on this
patch.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qapi-schema.json |  11 +++
 qmp-commands.hx  |  42 +++++++++++
 qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)

Comments

Fam Zheng Jan. 24, 2014, 10:48 a.m. UTC | #1
On Thu, 01/23 22:46, Amos Kong wrote:
> This patch introduces a new monitor command to query QMP schema
> information, the return data is a range of schema structs, which
> contains the useful metadata to help management to check supported
> features, QMP commands detail, etc.
> 
> We use qapi-introspect.py to parse all json definition in
> qapi-schema.json, and generate a range of dictionaries with metadata.
> The query command will visit the dictionaries and fill the data
> to allocated struct tree. Then QMP infrastructure will convert
> the tree to json string and return to QMP client.
> 
> TODO:
> Wenchao Xia is working to convert QMP events to qapi-schema.json,
> then event can also be queried by this interface.
> 
> I will introduce another command 'query-qga-schema' to query QGA
> schema information, it's easy to add this support based on this
> patch.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json |  11 +++
>  qmp-commands.hx  |  42 +++++++++++
>  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 268 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c63f0ca..6033383 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4411,3 +4411,14 @@
>      'reference-type': 'String',
>      'type': 'DataObjectType',
>      'unionobj': 'DataObjectUnion' } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# @returns: list of @DataObject
> +#
> +# Since: 1.8
> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 02cc815..b83762d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3291,6 +3291,48 @@ Example:
>     }
>  
>  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'
> +- "returns": return data of qmp command (json-object, optional)
> +
> +Example:
> +
> +-> { "execute": "query-qmp-schema" }
> +-> { "return": [
> +     {
> +         "name": "query-name",
> +         "type": "command",
> +         "returns": {
> +             "name": "NameInfo",
> +             "type": "type",
> +             "data": [
> +                 {
> +                     "name": "name",
> +                     "optional": true,
> +                     "recursive": false,
> +                     "type": "str"
> +                 }
> +             ]
> +         }
> +     }
> +  }
> +
> +EQMP
>  
>      {
>          .name       = "blockdev-add",
> diff --git a/qmp.c b/qmp.c
> index 0f46171..a64ae6d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -27,6 +27,8 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp-input-visitor.h"
>  #include "hw/boards.h"
> +#include "qapi/qmp/qjson.h"
> +#include "qapi-introspect.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>      return arch_query_cpu_definitions(errp);
>  }
>  
> +static strList *qobject_to_strlist(QObject *data)
> +{
> +    strList *list = NULL;
> +    strList **plist = &list;
> +    QList *qlist;
> +    const QListEntry *lent;
> +
> +    qlist = qobject_to_qlist(data);
> +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> +        strList *entry = g_malloc0(sizeof(strList));
> +        entry->value = g_strdup(qobject_get_str(lent->value));
> +        *plist = entry;
> +        plist = &entry->next;
> +    }
> +
> +    return list;
> +}
> +
> +static DataObject *qobject_to_dataobj(QObject *data);
> +
> +static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> +{
> +
> +    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> +
> +    member->type = g_malloc0(sizeof(DataObjectMemberType));
> +    if (data->type->code == QTYPE_QDICT) {
> +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> +        member->type->extend = qobject_to_dataobj(data);
> +    } else {
> +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> +        member->type->reference = g_strdup(qobject_get_str(data));
> +    }
> +
> +    return member;
> +}
> +
> +static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> +{
> +    DataObjectMemberList *list = NULL;
> +    DataObjectMemberList **plist = &list;
> +    QDict *qdict = qobject_to_qdict(data);
> +    const QDictEntry *dent;
> +
> +    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> +        entry->value = qobject_to_dataobjmem(dent->value);
> +
> +        entry->value->has_optional = true;
> +        entry->value->has_name = true;
> +        if (dent->key[0] == '*') {
> +            entry->value->optional = true;
> +            entry->value->name = g_strdup(dent->key + 1);
> +        } else {
> +            entry->value->name = g_strdup(dent->key);
> +        }
> +        *plist = entry;
> +        plist = &entry->next;
> +    }
> +
> +    return list;
> +}
> +
> +static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> +{
> +    const QListEntry *lent;
> +    DataObjectMemberList *list = NULL;
> +    DataObjectMemberList **plist = &list;
> +    QList *qlist = qobject_to_qlist(data);
> +
> +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> +        entry->value = qobject_to_dataobjmem(lent->value);
> +        entry->value->has_optional = true;
> +        entry->value->has_name = true;
> +        *plist = entry;
> +        plist = &entry->next;
> +    }
> +
> +    return list;
> +}
> +
> +static DataObjectMemberList *qobject_to_memlist(QObject *data)

This whole converting is cumbersome. You already did all the traversing through
the type jungle in python when generating this, it's not necessary to do the
similar thing again here.

Alternatively, I think we have a good reason to extend QMP framework as
necessary here, as we are doing "QMP introspection", which is a part of the
framework:

 * Define final output into qmp_schema_table[], no need to box it like:

        "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
        'ErrorClass', '_obj_data': {'data': ...

   just put it content of "qmp-introspection.output.txt" as a long string in
   the header, like you would generate in qobject_to_memlist:

        const char *qmp_schema_table =
        "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
        "{ 'name': '...', ...},"
        ...
        ;

 * Add a new type of qmp command, that returns a QString as a json literal.
   query-qmp-schema is defined as this type. (This wouldn't be much code, but
   may be abused in the future, I'm afraid. However we can review, limit its
   use to introspection only)

 * And return qmp_schema_table from query-qmp-shema, which will be copied to
   the wire.

Feel free to disagree, it's not a perfect solution. But I really think we need
to avoid duplicating "enum", "base", "type", "union", "discriminator", ...

Fam

> +{
> +    DataObjectMemberList *list = NULL;
> +    QDict *qdict = qobject_to_qdict(data);
> +    QObject *subdata = qdict_get(qdict, "_obj_data");
> +
> +    list = NULL;
> +    if (subdata->type->code == QTYPE_QDICT) {
> +        list = qobject_to_dict_memlist(subdata);
> +    } else if (subdata->type->code == QTYPE_QLIST) {
> +        list = qobject_to_list_memlist(subdata);
> +    }
> +
> +    return list;
> +}
> +
> +static DataObject *qobject_to_dataobj(QObject *data)
> +{
> +    QObject *subdata;
> +    QDict *qdict;
> +    const char *obj_type, *obj_recursive;
> +    DataObject *obj = g_malloc0(sizeof(DataObject));
> +
> +    if (data->type->code == QTYPE_QSTRING) {
> +        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> +        obj->reference_type = g_malloc0(sizeof(String));
> +        obj->reference_type->str = g_strdup(qobject_get_str(data));
> +        return obj;
> +    }
> +
> +    qdict = qobject_to_qdict(data);
> +    assert(qdict != NULL);
> +
> +    obj_type = qobject_get_str(qdict_get(qdict, "_obj_type"));
> +    obj_recursive = qobject_get_str(qdict_get(qdict, "_obj_recursive"));
> +    if (!strcmp(obj_recursive, "True")) {
> +        obj->has_recursive = true;
> +        obj->recursive = true;
> +    }
> +
> +    obj->has_name = true;
> +    obj->name = g_strdup(qobject_get_str(qdict_get(qdict, "_obj_name")));
> +
> +    subdata = qdict_get(qdict, "_obj_data");
> +    qdict = qobject_to_qdict(subdata);
> +
> +    if (!strcmp(obj_type, "command")) {
> +        obj->kind = DATA_OBJECT_KIND_COMMAND;
> +        obj->command = g_malloc0(sizeof(DataObjectCommand));
> +        subdata = qdict_get(qobject_to_qdict(subdata), "data");
> +
> +        if (subdata && subdata->type->code == QTYPE_QDICT) {
> +            obj->command->has_data = true;
> +            obj->command->data = qobject_to_memlist(subdata);
> +        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> +            abort();
> +        }
> +
> +        subdata = qdict_get(qdict, "returns");
> +        if (subdata) {
> +            obj->command->has_returns = true;
> +            obj->command->returns = qobject_to_dataobj(subdata);
> +        }
> +
> +        subdata = qdict_get(qdict, "gen");
> +        if (subdata && subdata->type->code == QTYPE_QSTRING) {
> +            obj->command->has_gen = true;
> +            if (!strcmp(qobject_get_str(subdata), "no")) {
> +                obj->command->gen = false;
> +            } else {
> +                obj->command->gen = true;
> +            }
> +        }
> +    } else if (!strcmp(obj_type, "union")) {
> +        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> +        obj->unionobj = g_malloc0(sizeof(DataObjectUnion));
> +        subdata = qdict_get(qdict, "data");
> +        obj->unionobj->data = qobject_to_memlist(subdata);
> +
> +        subdata = qdict_get(qdict, "base");
> +        if (subdata) {
> +            obj->unionobj->has_base = true;
> +            obj->unionobj->base = qobject_to_dataobj(subdata);
> +        }
> +
> +        subdata = qdict_get(qdict, "discriminator");
> +        if (subdata) {
> +            obj->unionobj->has_discriminator = true;
> +            obj->unionobj->discriminator = qobject_to_dataobj(subdata);
> +        }
> +    } else if (!strcmp(obj_type, "type")) {
> +        obj->kind = DATA_OBJECT_KIND_TYPE;
> +        obj->type = g_malloc0(sizeof(DataObjectType));
> +        subdata = qdict_get(qdict, "data");
> +        if (subdata) {
> +            obj->type->data = qobject_to_memlist(subdata);
> +        }
> +     } else if (!strcmp(obj_type, "enum")) {
> +        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> +        obj->enumeration = g_malloc0(sizeof(DataObjectEnumeration));
> +        subdata = qdict_get(qdict, "data");
> +        obj->enumeration->data = qobject_to_strlist(subdata);
> +    } else {
> +        obj->has_name = false;
> +        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> +        obj->anonymous_struct = g_malloc0(sizeof(DataObjectAnonymousStruct));
> +        obj->anonymous_struct->data = qobject_to_memlist(data);
> +    }
> +
> +    return obj;
> +}
> +
> +DataObjectList *qmp_query_qmp_schema(Error **errp)
> +{
> +    DataObjectList *list = NULL;
> +    DataObjectList **plist = &list;
> +    QObject *data;
> +    int i;
> +
> +    for (i = 0; qmp_schema_table[i]; i++) {
> +        data = qobject_from_json(qmp_schema_table[i]);
> +        assert(data != NULL);
> +        DataObjectList *entry = g_malloc0(sizeof(DataObjectList));
> +        entry->value = qobject_to_dataobj(data);
> +        *plist = entry;
> +        plist = &entry->next;
> +    }
> +
> +    return list;
> +}
> +
>  void qmp_add_client(const char *protocol, const char *fdname,
>                      bool has_skipauth, bool skipauth, bool has_tls, bool tls,
>                      Error **errp)
> -- 
> 1.8.4.2
> 
>
Amos Kong Jan. 27, 2014, 8:17 a.m. UTC | #2
CC Libvirt-list

Original discussion:
  http://marc.info/?l=qemu-devel&m=139048842504757&w=2
  [Qemu-devel] [PATCH v4 0/5] QMP full introspection

On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> On Thu, 01/23 22:46, Amos Kong wrote:
> > This patch introduces a new monitor command to query QMP schema
> > information, the return data is a range of schema structs, which
> > contains the useful metadata to help management to check supported
> > features, QMP commands detail, etc.
> > 
> > We use qapi-introspect.py to parse all json definition in
> > qapi-schema.json, and generate a range of dictionaries with metadata.
> > The query command will visit the dictionaries and fill the data
> > to allocated struct tree. Then QMP infrastructure will convert
> > the tree to json string and return to QMP client.
> > 
> > TODO:
> > Wenchao Xia is working to convert QMP events to qapi-schema.json,
> > then event can also be queried by this interface.
> > 
> > I will introduce another command 'query-qga-schema' to query QGA
> > schema information, it's easy to add this support based on this
> > patch.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  qapi-schema.json |  11 +++
> >  qmp-commands.hx  |  42 +++++++++++
> >  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 268 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index c63f0ca..6033383 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4411,3 +4411,14 @@
> >      'reference-type': 'String',
> >      'type': 'DataObjectType',
> >      'unionobj': 'DataObjectUnion' } }
> > +
> > +##
> > +# @query-qmp-schema
> > +#
> > +# Query QMP schema information
> > +#
> > +# @returns: list of @DataObject
> > +#
> > +# Since: 1.8
> > +##
> > +{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 02cc815..b83762d 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -3291,6 +3291,48 @@ Example:
> >     }
> >  
> >  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'
> > +- "returns": return data of qmp command (json-object, optional)
> > +
> > +Example:
> > +
> > +-> { "execute": "query-qmp-schema" }
> > +-> { "return": [
> > +     {
> > +         "name": "query-name",
> > +         "type": "command",
> > +         "returns": {
> > +             "name": "NameInfo",
> > +             "type": "type",
> > +             "data": [
> > +                 {
> > +                     "name": "name",
> > +                     "optional": true,
> > +                     "recursive": false,
> > +                     "type": "str"
> > +                 }
> > +             ]
> > +         }
> > +     }
> > +  }
> > +
> > +EQMP
> >  
> >      {
> >          .name       = "blockdev-add",
> > diff --git a/qmp.c b/qmp.c
> > index 0f46171..a64ae6d 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -27,6 +27,8 @@
> >  #include "qapi/qmp/qobject.h"
> >  #include "qapi/qmp-input-visitor.h"
> >  #include "hw/boards.h"
> > +#include "qapi/qmp/qjson.h"
> > +#include "qapi-introspect.h"
> >  
> >  NameInfo *qmp_query_name(Error **errp)
> >  {
> > @@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> >      return arch_query_cpu_definitions(errp);
> >  }
> >  
> > +static strList *qobject_to_strlist(QObject *data)
> > +{
> > +    strList *list = NULL;
> > +    strList **plist = &list;
> > +    QList *qlist;
> > +    const QListEntry *lent;
> > +
> > +    qlist = qobject_to_qlist(data);
> > +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > +        strList *entry = g_malloc0(sizeof(strList));
> > +        entry->value = g_strdup(qobject_get_str(lent->value));
> > +        *plist = entry;
> > +        plist = &entry->next;
> > +    }
> > +
> > +    return list;
> > +}
> > +
> > +static DataObject *qobject_to_dataobj(QObject *data);
> > +
> > +static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> > +{
> > +
> > +    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> > +
> > +    member->type = g_malloc0(sizeof(DataObjectMemberType));
> > +    if (data->type->code == QTYPE_QDICT) {
> > +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > +        member->type->extend = qobject_to_dataobj(data);
> > +    } else {
> > +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > +        member->type->reference = g_strdup(qobject_get_str(data));
> > +    }
> > +
> > +    return member;
> > +}
> > +
> > +static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> > +{
> > +    DataObjectMemberList *list = NULL;
> > +    DataObjectMemberList **plist = &list;
> > +    QDict *qdict = qobject_to_qdict(data);
> > +    const QDictEntry *dent;
> > +
> > +    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> > +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> > +        entry->value = qobject_to_dataobjmem(dent->value);
> > +
> > +        entry->value->has_optional = true;
> > +        entry->value->has_name = true;
> > +        if (dent->key[0] == '*') {
> > +            entry->value->optional = true;
> > +            entry->value->name = g_strdup(dent->key + 1);
> > +        } else {
> > +            entry->value->name = g_strdup(dent->key);
> > +        }
> > +        *plist = entry;
> > +        plist = &entry->next;
> > +    }
> > +
> > +    return list;
> > +}
> > +
> > +static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> > +{
> > +    const QListEntry *lent;
> > +    DataObjectMemberList *list = NULL;
> > +    DataObjectMemberList **plist = &list;
> > +    QList *qlist = qobject_to_qlist(data);
> > +
> > +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> > +        entry->value = qobject_to_dataobjmem(lent->value);
> > +        entry->value->has_optional = true;
> > +        entry->value->has_name = true;
> > +        *plist = entry;
> > +        plist = &entry->next;
> > +    }
> > +
> > +    return list;
> > +}
> > +
> > +static DataObjectMemberList *qobject_to_memlist(QObject *data)
> 
> This whole converting is cumbersome. You already did all the traversing through
> the type jungle in python when generating this, it's not necessary to do the
> similar thing again here.

We can parse raw schemas and generate json string table, we can't
directly return the string / qobject to monitor, C code has to convert
the json to qobject, we have to revisit the qobject and convert them
to DataObject/DataObjectMember/DataObject... structs.
 
> Alternatively, I think we have a good reason to extend QMP framework as
> necessary here, as we are doing "QMP introspection", which is a part of the
> framework:
> 
>  * Define final output into qmp_schema_table[], no need to box it like:
> 
>         "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
>         'ErrorClass', '_obj_data': {'data': ...
> 
>    just put it content of "qmp-introspection.output.txt" as a long string in
>    the header,



>    like you would generate in qobject_to_memlist:
> 
>         const char *qmp_schema_table =
>         "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
>         "{ 'name': '...', ...},"

The keys are used for metadata might be 'recursive', 'optional', etc.
It might exists problem in namespace, let's use '_obj_' or '_' prefix
for the metadata keys.

I used a nested dictionary to describe a DataObject, because we can
store the metadata and definition to different level, it's helpful
in parse the output by Libvirt.

 example:
   "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"

It's good to store _type, _name, data to same level, but the metadata
of items of data's value dictionary can't be appended to same level.

    "{ '_name': 'NameInfo', '_type': 'type',
        'data': {
                  'name': 'str', '_name_optional': 'True',
                  'job': 'str', '_job_optional': 'True'
                }
     }"


A better solution, but I don't know if it will cause trouble for
Libvirt to parse the output.

    "{'_type': 'type', '_name': 'NameInfo',
        'data': {  'job': {'_value': 'str', '_recursive': 'True'},
                   'name': {'_value': 'str', '_recursive': 'True'}
                },
        '_recursive': 'False'
     }"

When we describe a DataObject (dict/list/str, one schema, extened
schema, schema member, etc), so I we generally use a nested dictionary
to describe a DataObject, it will split the metadata with original
data two different dictionary level, it's convenient for parse of
Libvirt. Here I just use a dict member as an example, actually
it's more complex to parse all kinds of data objects.

>         ...
>         ;
> 
>  * Add a new type of qmp command, that returns a QString as a json literal.
>    query-qmp-schema is defined as this type. (This wouldn't be much code, but
>    may be abused in the future, I'm afraid. However we can review, limit its
>    use to introspection only)
> 
>  * And return qmp_schema_table from query-qmp-shema, which will be copied to
>    the wire.
> 
> Feel free to disagree, it's not a perfect solution. But I really think we need
> to avoid duplicating "enum", "base", "type", "union", "discriminator", ...


In the past, we didn't consider to extend and add metadata by Python, so
Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
But now, we already successfully to transfer this work to Python, and
we can adjust to make management to be happy for the metadata and
format.

The problem is that Libvirt should parse the output twice, the json
items are also json object string. 

Eric, Is it acceptabled?

  Example:
  * as suggested by Fam to put the metadta with schema data in same
    dict level
  * process some special cases by nested dictionary
    (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
  * return a strList to Libvirt, the content of string is json object,
    that contains the metadata.

{
    "return": [
        "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}", 
        "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}", 
        "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}", 
        "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}", 
        "......",
        "......",
        "......"
    ]
}

> Fam
Amos Kong Jan. 27, 2014, 8:50 a.m. UTC | #3
On Mon, Jan 27, 2014 at 04:17:56PM +0800, Amos Kong wrote:
> CC Libvirt-list
> 
> Original discussion:
>   http://marc.info/?l=qemu-devel&m=139048842504757&w=2
>   [Qemu-devel] [PATCH v4 0/5] QMP full introspection
> 
> On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> > On Thu, 01/23 22:46, Amos Kong wrote:
> > > This patch introduces a new monitor command to query QMP schema
> > > information, the return data is a range of schema structs, which
> > > contains the useful metadata to help management to check supported
> > > features, QMP commands detail, etc.
> > > 
> > > We use qapi-introspect.py to parse all json definition in
> > > qapi-schema.json, and generate a range of dictionaries with metadata.
> > > The query command will visit the dictionaries and fill the data
> > > to allocated struct tree. Then QMP infrastructure will convert
> > > the tree to json string and return to QMP client.
> > > 
> > > TODO:
> > > Wenchao Xia is working to convert QMP events to qapi-schema.json,
> > > then event can also be queried by this interface.
> > > 
> > > I will introduce another command 'query-qga-schema' to query QGA
> > > schema information, it's easy to add this support based on this
> > > patch.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  qapi-schema.json |  11 +++
> > >  qmp-commands.hx  |  42 +++++++++++
> > >  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 268 insertions(+)


> > > +static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> > > +{
> > > +    const QListEntry *lent;
> > > +    DataObjectMemberList *list = NULL;
> > > +    DataObjectMemberList **plist = &list;
> > > +    QList *qlist = qobject_to_qlist(data);
> > > +
> > > +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > > +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> > > +        entry->value = qobject_to_dataobjmem(lent->value);
> > > +        entry->value->has_optional = true;
> > > +        entry->value->has_name = true;
> > > +        *plist = entry;
> > > +        plist = &entry->next;
> > > +    }
> > > +
> > > +    return list;
> > > +}
> > > +
> > > +static DataObjectMemberList *qobject_to_memlist(QObject *data)
> > 
> > This whole converting is cumbersome. You already did all the traversing through
> > the type jungle in python when generating this, it's not necessary to do the
> > similar thing again here.
> 
> We can parse raw schemas and generate json string table, we can't
> directly return the string / qobject to monitor, C code has to convert
> the json to qobject, we have to revisit the qobject and convert them
> to DataObject/DataObjectMember/DataObject... structs.
>  
> > Alternatively, I think we have a good reason to extend QMP framework as
> > necessary here, as we are doing "QMP introspection", which is a part of the
> > framework:
> > 
> >  * Define final output into qmp_schema_table[], no need to box it like:
> > 
> >         "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
> >         'ErrorClass', '_obj_data': {'data': ...
> > 
> >    just put it content of "qmp-introspection.output.txt" as a long string in
> >    the header,
> 
> 
> 
> >    like you would generate in qobject_to_memlist:
> > 
> >         const char *qmp_schema_table =
> >         "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
> >         "{ 'name': '...', ...},"
> 
> The keys are used for metadata might be 'recursive', 'optional', etc.
> It might exists problem in namespace, let's use '_obj_' or '_' prefix
> for the metadata keys.
> 
> I used a nested dictionary to describe a DataObject, because we can
> store the metadata and definition to different level, it's helpful
> in parse the output by Libvirt.
> 
>  example:
>    "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
> 
> It's good to store _type, _name, data to same level, but the metadata
> of items of data's value dictionary can't be appended to same level.
> 
>     "{ '_name': 'NameInfo', '_type': 'type',
>         'data': {
>                   'name': 'str', '_name_optional': 'True',
>                   'job': 'str', '_job_optional': 'True'
>                 }
>      }"
> 
> 
> A better solution, but I don't know if it will cause trouble for
> Libvirt to parse the output.
> 
>     "{'_type': 'type', '_name': 'NameInfo',
>         'data': {  'job': {'_value': 'str', '_recursive': 'True'},
>                    'name': {'_value': 'str', '_recursive': 'True'}
>                 },
>         '_recursive': 'False'
>      }"
> 
> When we describe a DataObject (dict/list/str, one schema, extened
> schema, schema member, etc), so I we generally use a nested dictionary
> to describe a DataObject, it will split the metadata with original
> data two different dictionary level, it's convenient for parse of
> Libvirt. Here I just use a dict member as an example, actually
> it's more complex to parse all kinds of data objects.
> 
> >         ...
> >         ;
> > 
> >  * Add a new type of qmp command, that returns a QString as a json literal.
> >    query-qmp-schema is defined as this type. (This wouldn't be much code, but
> >    may be abused in the future, I'm afraid. However we can review, limit its
> >    use to introspection only)
> > 
> >  * And return qmp_schema_table from query-qmp-shema, which will be copied to
> >    the wire.
> > 
> > Feel free to disagree, it's not a perfect solution. But I really think we need
> > to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
> 
> 
> In the past, we didn't consider to extend and add metadata by Python, so
> Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> But now, we already successfully to transfer this work to Python, and
> we can adjust to make management to be happy for the metadata and
> format.
> 
> The problem is that Libvirt should parse the output twice, the json
> items are also json object string. 
> 
> Eric, Is it acceptabled?
> 
>   Example:
>   * as suggested by Fam to put the metadta with schema data in same
>     dict level
>   * process some special cases by nested dictionary
>     (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
>   * return a strList to Libvirt, the content of string is json object,
>     that contains the metadata.
> 
> {
>     "return": [
>         "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}", 
>         "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}", 
>         "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}", 
>         "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}", 
>         "......",
>         "......",
>         "......"
>     ]
> }

Provide two txt files:
 https://i-kvm.rhcloud.com/static/pub/v5/qapi-introspect.h
 https://i-kvm.rhcloud.com/static/pub/v5/query-output.txt
Paolo Bonzini Jan. 27, 2014, 9:38 a.m. UTC | #4
Il 27/01/2014 09:17, Amos Kong ha scritto:
> CC Libvirt-list
>
> Original discussion:
>   http://marc.info/?l=qemu-devel&m=139048842504757&w=2
>   [Qemu-devel] [PATCH v4 0/5] QMP full introspection
>
> On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
>> On Thu, 01/23 22:46, Amos Kong wrote:
>>> This patch introduces a new monitor command to query QMP schema
>>> information, the return data is a range of schema structs, which
>>> contains the useful metadata to help management to check supported
>>> features, QMP commands detail, etc.
>>>
>>> We use qapi-introspect.py to parse all json definition in
>>> qapi-schema.json, and generate a range of dictionaries with metadata.
>>> The query command will visit the dictionaries and fill the data
>>> to allocated struct tree. Then QMP infrastructure will convert
>>> the tree to json string and return to QMP client.
>>>
>>> TODO:
>>> Wenchao Xia is working to convert QMP events to qapi-schema.json,
>>> then event can also be queried by this interface.
>>>
>>> I will introduce another command 'query-qga-schema' to query QGA
>>> schema information, it's easy to add this support based on this
>>> patch.
>>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> ---
>>>  qapi-schema.json |  11 +++
>>>  qmp-commands.hx  |  42 +++++++++++
>>>  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 268 insertions(+)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index c63f0ca..6033383 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -4411,3 +4411,14 @@
>>>      'reference-type': 'String',
>>>      'type': 'DataObjectType',
>>>      'unionobj': 'DataObjectUnion' } }
>>> +
>>> +##
>>> +# @query-qmp-schema
>>> +#
>>> +# Query QMP schema information
>>> +#
>>> +# @returns: list of @DataObject
>>> +#
>>> +# Since: 1.8
>>> +##
>>> +{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 02cc815..b83762d 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -3291,6 +3291,48 @@ Example:
>>>     }
>>>
>>>  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'
>>> +- "returns": return data of qmp command (json-object, optional)
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "query-qmp-schema" }
>>> +-> { "return": [
>>> +     {
>>> +         "name": "query-name",
>>> +         "type": "command",
>>> +         "returns": {
>>> +             "name": "NameInfo",
>>> +             "type": "type",
>>> +             "data": [
>>> +                 {
>>> +                     "name": "name",
>>> +                     "optional": true,
>>> +                     "recursive": false,
>>> +                     "type": "str"
>>> +                 }
>>> +             ]
>>> +         }
>>> +     }
>>> +  }
>>> +
>>> +EQMP
>>>
>>>      {
>>>          .name       = "blockdev-add",
>>> diff --git a/qmp.c b/qmp.c
>>> index 0f46171..a64ae6d 100644
>>> --- a/qmp.c
>>> +++ b/qmp.c
>>> @@ -27,6 +27,8 @@
>>>  #include "qapi/qmp/qobject.h"
>>>  #include "qapi/qmp-input-visitor.h"
>>>  #include "hw/boards.h"
>>> +#include "qapi/qmp/qjson.h"
>>> +#include "qapi-introspect.h"
>>>
>>>  NameInfo *qmp_query_name(Error **errp)
>>>  {
>>> @@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>>>      return arch_query_cpu_definitions(errp);
>>>  }
>>>
>>> +static strList *qobject_to_strlist(QObject *data)
>>> +{
>>> +    strList *list = NULL;
>>> +    strList **plist = &list;
>>> +    QList *qlist;
>>> +    const QListEntry *lent;
>>> +
>>> +    qlist = qobject_to_qlist(data);
>>> +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
>>> +        strList *entry = g_malloc0(sizeof(strList));
>>> +        entry->value = g_strdup(qobject_get_str(lent->value));
>>> +        *plist = entry;
>>> +        plist = &entry->next;
>>> +    }
>>> +
>>> +    return list;
>>> +}
>>> +
>>> +static DataObject *qobject_to_dataobj(QObject *data);
>>> +
>>> +static DataObjectMember *qobject_to_dataobjmem(QObject *data)
>>> +{
>>> +
>>> +    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
>>> +
>>> +    member->type = g_malloc0(sizeof(DataObjectMemberType));
>>> +    if (data->type->code == QTYPE_QDICT) {
>>> +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
>>> +        member->type->extend = qobject_to_dataobj(data);
>>> +    } else {
>>> +        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
>>> +        member->type->reference = g_strdup(qobject_get_str(data));
>>> +    }
>>> +
>>> +    return member;
>>> +}
>>> +
>>> +static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
>>> +{
>>> +    DataObjectMemberList *list = NULL;
>>> +    DataObjectMemberList **plist = &list;
>>> +    QDict *qdict = qobject_to_qdict(data);
>>> +    const QDictEntry *dent;
>>> +
>>> +    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
>>> +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
>>> +        entry->value = qobject_to_dataobjmem(dent->value);
>>> +
>>> +        entry->value->has_optional = true;
>>> +        entry->value->has_name = true;
>>> +        if (dent->key[0] == '*') {
>>> +            entry->value->optional = true;
>>> +            entry->value->name = g_strdup(dent->key + 1);
>>> +        } else {
>>> +            entry->value->name = g_strdup(dent->key);
>>> +        }
>>> +        *plist = entry;
>>> +        plist = &entry->next;
>>> +    }
>>> +
>>> +    return list;
>>> +}
>>> +
>>> +static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
>>> +{
>>> +    const QListEntry *lent;
>>> +    DataObjectMemberList *list = NULL;
>>> +    DataObjectMemberList **plist = &list;
>>> +    QList *qlist = qobject_to_qlist(data);
>>> +
>>> +    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
>>> +        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
>>> +        entry->value = qobject_to_dataobjmem(lent->value);
>>> +        entry->value->has_optional = true;
>>> +        entry->value->has_name = true;
>>> +        *plist = entry;
>>> +        plist = &entry->next;
>>> +    }
>>> +
>>> +    return list;
>>> +}
>>> +
>>> +static DataObjectMemberList *qobject_to_memlist(QObject *data)
>>
>> This whole converting is cumbersome. You already did all the traversing through
>> the type jungle in python when generating this, it's not necessary to do the
>> similar thing again here.
>
> We can parse raw schemas and generate json string table, we can't
> directly return the string / qobject to monitor, C code has to convert
> the json to qobject, we have to revisit the qobject and convert them
> to DataObject/DataObjectMember/DataObject... structs.
>
>> Alternatively, I think we have a good reason to extend QMP framework as
>> necessary here, as we are doing "QMP introspection", which is a part of the
>> framework:
>>
>>  * Define final output into qmp_schema_table[], no need to box it like:
>>
>>         "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
>>         'ErrorClass', '_obj_data': {'data': ...
>>
>>    just put it content of "qmp-introspection.output.txt" as a long string in
>>    the header,
>
>
>
>>    like you would generate in qobject_to_memlist:
>>
>>         const char *qmp_schema_table =
>>         "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
>>         "{ 'name': '...', ...},"
>
> The keys are used for metadata might be 'recursive', 'optional', etc.
> It might exists problem in namespace, let's use '_obj_' or '_' prefix
> for the metadata keys.
>
> I used a nested dictionary to describe a DataObject, because we can
> store the metadata and definition to different level, it's helpful
> in parse the output by Libvirt.
>
>  example:
>    "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
>
> It's good to store _type, _name, data to same level, but the metadata
> of items of data's value dictionary can't be appended to same level.
>
>     "{ '_name': 'NameInfo', '_type': 'type',
>         'data': {
>                   'name': 'str', '_name_optional': 'True',
>                   'job': 'str', '_job_optional': 'True'
>                 }
>      }"
>
>
> A better solution, but I don't know if it will cause trouble for
> Libvirt to parse the output.
>
>     "{'_type': 'type', '_name': 'NameInfo',
>         'data': {  'job': {'_value': 'str', '_recursive': 'True'},
>                    'name': {'_value': 'str', '_recursive': 'True'}
>                 },
>         '_recursive': 'False'
>      }"
>
> When we describe a DataObject (dict/list/str, one schema, extened
> schema, schema member, etc), so I we generally use a nested dictionary
> to describe a DataObject, it will split the metadata with original
> data two different dictionary level, it's convenient for parse of
> Libvirt. Here I just use a dict member as an example, actually
> it's more complex to parse all kinds of data objects.
>
>>         ...
>>         ;
>>
>>  * Add a new type of qmp command, that returns a QString as a json literal.
>>    query-qmp-schema is defined as this type. (This wouldn't be much code, but
>>    may be abused in the future, I'm afraid. However we can review, limit its
>>    use to introspection only)
>>
>>  * And return qmp_schema_table from query-qmp-shema, which will be copied to
>>    the wire.
>>
>> Feel free to disagree, it's not a perfect solution. But I really think we need
>> to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
>
>
> In the past, we didn't consider to extend and add metadata by Python, so
> Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> But now, we already successfully to transfer this work to Python, and
> we can adjust to make management to be happy for the metadata and
> format.
>
> The problem is that Libvirt should parse the output twice, the json
> items are also json object string.
>
> Eric, Is it acceptabled?
>
>   Example:
>   * as suggested by Fam to put the metadta with schema data in same
>     dict level
>   * process some special cases by nested dictionary
>     (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
>   * return a strList to Libvirt, the content of string is json object,
>     that contains the metadata.
>
> {
>     "return": [
>         "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}",
>         "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}",
>         "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}",
>         "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}",
>         "......",
>         "......",
>         "......"
>     ]
> }
>
>> Fam
>

No, I don't like this.

QAPI types are perfectly able to "describe themselves", there is no need 
to escape to JSON.

Let's just do what this series is doing, minus the unnecessary recursion.

Paolo
Amos Kong Jan. 27, 2014, 10:07 a.m. UTC | #5
On Mon, Jan 27, 2014 at 10:38:24AM +0100, Paolo Bonzini wrote:
> Il 27/01/2014 09:17, Amos Kong ha scritto:
> >CC Libvirt-list
> >
> >Original discussion:
> >  http://marc.info/?l=qemu-devel&m=139048842504757&w=2
> >  [Qemu-devel] [PATCH v4 0/5] QMP full introspection
> >
> >On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> >>On Thu, 01/23 22:46, Amos Kong wrote:
> >>>This patch introduces a new monitor command to query QMP schema
> >>>information, the return data is a range of schema structs, which
> >>>contains the useful metadata to help management to check supported
> >>>features, QMP commands detail, etc.
> >>>
> >>>We use qapi-introspect.py to parse all json definition in
> >>>qapi-schema.json, and generate a range of dictionaries with metadata.
> >>>The query command will visit the dictionaries and fill the data
> >>>to allocated struct tree. Then QMP infrastructure will convert
> >>>the tree to json string and return to QMP client.
> >>>
> >>>TODO:
> >>>Wenchao Xia is working to convert QMP events to qapi-schema.json,
> >>>then event can also be queried by this interface.
> >>>
> >>>I will introduce another command 'query-qga-schema' to query QGA
> >>>schema information, it's easy to add this support based on this
> >>>patch.
> >>>
> >>>Signed-off-by: Amos Kong <akong@redhat.com>
> >>>---
> >>> qapi-schema.json |  11 +++
> >>> qmp-commands.hx  |  42 +++++++++++
> >>> qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 268 insertions(+)
> >>>
> >>>diff --git a/qapi-schema.json b/qapi-schema.json
> >>>index c63f0ca..6033383 100644
> >>>--- a/qapi-schema.json
> >>>+++ b/qapi-schema.json
> >>>@@ -4411,3 +4411,14 @@
> >>>     'reference-type': 'String',
> >>>     'type': 'DataObjectType',
> >>>     'unionobj': 'DataObjectUnion' } }
> >>>+
> >>>+##
> >>>+# @query-qmp-schema
> >>>+#
> >>>+# Query QMP schema information
> >>>+#
> >>>+# @returns: list of @DataObject
> >>>+#
> >>>+# Since: 1.8
> >>>+##
> >>>+{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> >>>diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>>index 02cc815..b83762d 100644
> >>>--- a/qmp-commands.hx
> >>>+++ b/qmp-commands.hx
> >>>@@ -3291,6 +3291,48 @@ Example:
> >>>    }
> >>>
> >>> 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'
> >>>+- "returns": return data of qmp command (json-object, optional)
> >>>+
> >>>+Example:
> >>>+
> >>>+-> { "execute": "query-qmp-schema" }
> >>>+-> { "return": [
> >>>+     {
> >>>+         "name": "query-name",
> >>>+         "type": "command",
> >>>+         "returns": {
> >>>+             "name": "NameInfo",
> >>>+             "type": "type",
> >>>+             "data": [
> >>>+                 {
> >>>+                     "name": "name",
> >>>+                     "optional": true,
> >>>+                     "recursive": false,
> >>>+                     "type": "str"
> >>>+                 }
> >>>+             ]
> >>>+         }
> >>>+     }
> >>>+  }
> >>>+
> >>>+EQMP
> >>>
> >>>     {
> >>>         .name       = "blockdev-add",
> >>>diff --git a/qmp.c b/qmp.c
> >>>index 0f46171..a64ae6d 100644
> >>>--- a/qmp.c
> >>>+++ b/qmp.c
> >>>@@ -27,6 +27,8 @@
> >>> #include "qapi/qmp/qobject.h"
> >>> #include "qapi/qmp-input-visitor.h"
> >>> #include "hw/boards.h"
> >>>+#include "qapi/qmp/qjson.h"
> >>>+#include "qapi-introspect.h"
> >>>
> >>> NameInfo *qmp_query_name(Error **errp)
> >>> {
> >>>@@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> >>>     return arch_query_cpu_definitions(errp);
> >>> }
> >>>
> >>>+static strList *qobject_to_strlist(QObject *data)
> >>>+{
> >>>+    strList *list = NULL;
> >>>+    strList **plist = &list;
> >>>+    QList *qlist;
> >>>+    const QListEntry *lent;
> >>>+
> >>>+    qlist = qobject_to_qlist(data);
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        strList *entry = g_malloc0(sizeof(strList));
> >>>+        entry->value = g_strdup(qobject_get_str(lent->value));
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObject *qobject_to_dataobj(QObject *data);
> >>>+
> >>>+static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> >>>+{
> >>>+
> >>>+    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> >>>+
> >>>+    member->type = g_malloc0(sizeof(DataObjectMemberType));
> >>>+    if (data->type->code == QTYPE_QDICT) {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >>>+        member->type->extend = qobject_to_dataobj(data);
> >>>+    } else {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> >>>+        member->type->reference = g_strdup(qobject_get_str(data));
> >>>+    }
> >>>+
> >>>+    return member;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> >>>+{
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QDict *qdict = qobject_to_qdict(data);
> >>>+    const QDictEntry *dent;
> >>>+
> >>>+    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(dent->value);
> >>>+
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        if (dent->key[0] == '*') {
> >>>+            entry->value->optional = true;
> >>>+            entry->value->name = g_strdup(dent->key + 1);
> >>>+        } else {
> >>>+            entry->value->name = g_strdup(dent->key);
> >>>+        }
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> >>>+{
> >>>+    const QListEntry *lent;
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QList *qlist = qobject_to_qlist(data);
> >>>+
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(lent->value);
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_memlist(QObject *data)
> >>
> >>This whole converting is cumbersome. You already did all the traversing through
> >>the type jungle in python when generating this, it's not necessary to do the
> >>similar thing again here.
> >
> >We can parse raw schemas and generate json string table, we can't
> >directly return the string / qobject to monitor, C code has to convert
> >the json to qobject, we have to revisit the qobject and convert them
> >to DataObject/DataObjectMember/DataObject... structs.
> >
> >>Alternatively, I think we have a good reason to extend QMP framework as
> >>necessary here, as we are doing "QMP introspection", which is a part of the
> >>framework:
> >>
> >> * Define final output into qmp_schema_table[], no need to box it like:
> >>
> >>        "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
> >>        'ErrorClass', '_obj_data': {'data': ...
> >>
> >>   just put it content of "qmp-introspection.output.txt" as a long string in
> >>   the header,
> >
> >
> >
> >>   like you would generate in qobject_to_memlist:
> >>
> >>        const char *qmp_schema_table =
> >>        "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
> >>        "{ 'name': '...', ...},"
> >
> >The keys are used for metadata might be 'recursive', 'optional', etc.
> >It might exists problem in namespace, let's use '_obj_' or '_' prefix
> >for the metadata keys.
> >
> >I used a nested dictionary to describe a DataObject, because we can
> >store the metadata and definition to different level, it's helpful
> >in parse the output by Libvirt.
> >
> > example:
> >   "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
> >
> >It's good to store _type, _name, data to same level, but the metadata
> >of items of data's value dictionary can't be appended to same level.
> >
> >    "{ '_name': 'NameInfo', '_type': 'type',
> >        'data': {
> >                  'name': 'str', '_name_optional': 'True',
> >                  'job': 'str', '_job_optional': 'True'
> >                }
> >     }"
> >
> >
> >A better solution, but I don't know if it will cause trouble for
> >Libvirt to parse the output.
> >
> >    "{'_type': 'type', '_name': 'NameInfo',
> >        'data': {  'job': {'_value': 'str', '_recursive': 'True'},
> >                   'name': {'_value': 'str', '_recursive': 'True'}
> >                },
> >        '_recursive': 'False'
> >     }"
> >
> >When we describe a DataObject (dict/list/str, one schema, extened
> >schema, schema member, etc), so I we generally use a nested dictionary
> >to describe a DataObject, it will split the metadata with original
> >data two different dictionary level, it's convenient for parse of
> >Libvirt. Here I just use a dict member as an example, actually
> >it's more complex to parse all kinds of data objects.
> >
> >>        ...
> >>        ;
> >>
> >> * Add a new type of qmp command, that returns a QString as a json literal.
> >>   query-qmp-schema is defined as this type. (This wouldn't be much code, but
> >>   may be abused in the future, I'm afraid. However we can review, limit its
> >>   use to introspection only)
> >>
> >> * And return qmp_schema_table from query-qmp-shema, which will be copied to
> >>   the wire.
> >>
> >>Feel free to disagree, it's not a perfect solution. But I really think we need
> >>to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
> >
> >
> >In the past, we didn't consider to extend and add metadata by Python, so
> >Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> >But now, we already successfully to transfer this work to Python, and
> >we can adjust to make management to be happy for the metadata and
> >format.
> >
> >The problem is that Libvirt should parse the output twice, the json
> >items are also json object string.
> >
> >Eric, Is it acceptabled?
> >
> >  Example:
> >  * as suggested by Fam to put the metadta with schema data in same
> >    dict level
> >  * process some special cases by nested dictionary
> >    (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
> >  * return a strList to Libvirt, the content of string is json object,
> >    that contains the metadata.
> >
> >{
> >    "return": [
> >        "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}",
> >        "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}",
> >        "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}",
> >        "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}",
> >        "......",
> >        "......",
> >        "......"
> >    ]
> >}
> >
> >>Fam
> >
> 
> No, I don't like this.
> 
> QAPI types are perfectly able to "describe themselves", there is no
> need to escape to JSON.
> 
> Let's just do what this series is doing, minus the unnecessary recursion.
 
The recursion extend was requested by Libvirt, they want to get a
extended output with metadata to avoid heavy parsing in Libvirt.

Let's see the response of libvirt people.

> Paolo
Paolo Bonzini Jan. 27, 2014, 10:15 a.m. UTC | #6
Il 27/01/2014 11:07, Amos Kong ha scritto:
>> > No, I don't like this.
>> >
>> > QAPI types are perfectly able to "describe themselves", there is no
>> > need to escape to JSON.
>> >
>> > Let's just do what this series is doing, minus the unnecessary recursion.
>
> The recursion extend was requested by Libvirt, they want to get a
> extended output with metadata to avoid heavy parsing in Libvirt.
>
> Let's see the response of libvirt people.
>

I think Eric already answered.

In any case, 4 seconds to produce 1.7 MB is a sign of a bug in my 
opinion.  What does the profile say?

Paolo
Fam Zheng Jan. 27, 2014, 10:46 a.m. UTC | #7
On Mon, 01/27 10:38, Paolo Bonzini wrote:
> Il 27/01/2014 09:17, Amos Kong ha scritto:
> >CC Libvirt-list
> >
> >Original discussion:
> >  http://marc.info/?l=qemu-devel&m=139048842504757&w=2
> >  [Qemu-devel] [PATCH v4 0/5] QMP full introspection
> >
> >On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> >>On Thu, 01/23 22:46, Amos Kong wrote:
> >>>This patch introduces a new monitor command to query QMP schema
> >>>information, the return data is a range of schema structs, which
> >>>contains the useful metadata to help management to check supported
> >>>features, QMP commands detail, etc.
> >>>
> >>>We use qapi-introspect.py to parse all json definition in
> >>>qapi-schema.json, and generate a range of dictionaries with metadata.
> >>>The query command will visit the dictionaries and fill the data
> >>>to allocated struct tree. Then QMP infrastructure will convert
> >>>the tree to json string and return to QMP client.
> >>>
> >>>TODO:
> >>>Wenchao Xia is working to convert QMP events to qapi-schema.json,
> >>>then event can also be queried by this interface.
> >>>
> >>>I will introduce another command 'query-qga-schema' to query QGA
> >>>schema information, it's easy to add this support based on this
> >>>patch.
> >>>
> >>>Signed-off-by: Amos Kong <akong@redhat.com>
> >>>---
> >>> qapi-schema.json |  11 +++
> >>> qmp-commands.hx  |  42 +++++++++++
> >>> qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 268 insertions(+)
> >>>
> >>>diff --git a/qapi-schema.json b/qapi-schema.json
> >>>index c63f0ca..6033383 100644
> >>>--- a/qapi-schema.json
> >>>+++ b/qapi-schema.json
> >>>@@ -4411,3 +4411,14 @@
> >>>     'reference-type': 'String',
> >>>     'type': 'DataObjectType',
> >>>     'unionobj': 'DataObjectUnion' } }
> >>>+
> >>>+##
> >>>+# @query-qmp-schema
> >>>+#
> >>>+# Query QMP schema information
> >>>+#
> >>>+# @returns: list of @DataObject
> >>>+#
> >>>+# Since: 1.8
> >>>+##
> >>>+{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> >>>diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>>index 02cc815..b83762d 100644
> >>>--- a/qmp-commands.hx
> >>>+++ b/qmp-commands.hx
> >>>@@ -3291,6 +3291,48 @@ Example:
> >>>    }
> >>>
> >>> 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'
> >>>+- "returns": return data of qmp command (json-object, optional)
> >>>+
> >>>+Example:
> >>>+
> >>>+-> { "execute": "query-qmp-schema" }
> >>>+-> { "return": [
> >>>+     {
> >>>+         "name": "query-name",
> >>>+         "type": "command",
> >>>+         "returns": {
> >>>+             "name": "NameInfo",
> >>>+             "type": "type",
> >>>+             "data": [
> >>>+                 {
> >>>+                     "name": "name",
> >>>+                     "optional": true,
> >>>+                     "recursive": false,
> >>>+                     "type": "str"
> >>>+                 }
> >>>+             ]
> >>>+         }
> >>>+     }
> >>>+  }
> >>>+
> >>>+EQMP
> >>>
> >>>     {
> >>>         .name       = "blockdev-add",
> >>>diff --git a/qmp.c b/qmp.c
> >>>index 0f46171..a64ae6d 100644
> >>>--- a/qmp.c
> >>>+++ b/qmp.c
> >>>@@ -27,6 +27,8 @@
> >>> #include "qapi/qmp/qobject.h"
> >>> #include "qapi/qmp-input-visitor.h"
> >>> #include "hw/boards.h"
> >>>+#include "qapi/qmp/qjson.h"
> >>>+#include "qapi-introspect.h"
> >>>
> >>> NameInfo *qmp_query_name(Error **errp)
> >>> {
> >>>@@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> >>>     return arch_query_cpu_definitions(errp);
> >>> }
> >>>
> >>>+static strList *qobject_to_strlist(QObject *data)
> >>>+{
> >>>+    strList *list = NULL;
> >>>+    strList **plist = &list;
> >>>+    QList *qlist;
> >>>+    const QListEntry *lent;
> >>>+
> >>>+    qlist = qobject_to_qlist(data);
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        strList *entry = g_malloc0(sizeof(strList));
> >>>+        entry->value = g_strdup(qobject_get_str(lent->value));
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObject *qobject_to_dataobj(QObject *data);
> >>>+
> >>>+static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> >>>+{
> >>>+
> >>>+    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> >>>+
> >>>+    member->type = g_malloc0(sizeof(DataObjectMemberType));
> >>>+    if (data->type->code == QTYPE_QDICT) {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> >>>+        member->type->extend = qobject_to_dataobj(data);
> >>>+    } else {
> >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> >>>+        member->type->reference = g_strdup(qobject_get_str(data));
> >>>+    }
> >>>+
> >>>+    return member;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> >>>+{
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QDict *qdict = qobject_to_qdict(data);
> >>>+    const QDictEntry *dent;
> >>>+
> >>>+    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(dent->value);
> >>>+
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        if (dent->key[0] == '*') {
> >>>+            entry->value->optional = true;
> >>>+            entry->value->name = g_strdup(dent->key + 1);
> >>>+        } else {
> >>>+            entry->value->name = g_strdup(dent->key);
> >>>+        }
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> >>>+{
> >>>+    const QListEntry *lent;
> >>>+    DataObjectMemberList *list = NULL;
> >>>+    DataObjectMemberList **plist = &list;
> >>>+    QList *qlist = qobject_to_qlist(data);
> >>>+
> >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> >>>+        entry->value = qobject_to_dataobjmem(lent->value);
> >>>+        entry->value->has_optional = true;
> >>>+        entry->value->has_name = true;
> >>>+        *plist = entry;
> >>>+        plist = &entry->next;
> >>>+    }
> >>>+
> >>>+    return list;
> >>>+}
> >>>+
> >>>+static DataObjectMemberList *qobject_to_memlist(QObject *data)
> >>
> >>This whole converting is cumbersome. You already did all the traversing through
> >>the type jungle in python when generating this, it's not necessary to do the
> >>similar thing again here.
> >
> >We can parse raw schemas and generate json string table, we can't
> >directly return the string / qobject to monitor, C code has to convert
> >the json to qobject, we have to revisit the qobject and convert them
> >to DataObject/DataObjectMember/DataObject... structs.
> >
> >>Alternatively, I think we have a good reason to extend QMP framework as
> >>necessary here, as we are doing "QMP introspection", which is a part of the
> >>framework:
> >>
> >> * Define final output into qmp_schema_table[], no need to box it like:
> >>
> >>        "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
> >>        'ErrorClass', '_obj_data': {'data': ...
> >>
> >>   just put it content of "qmp-introspection.output.txt" as a long string in
> >>   the header,
> >
> >
> >
> >>   like you would generate in qobject_to_memlist:
> >>
> >>        const char *qmp_schema_table =
> >>        "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
> >>        "{ 'name': '...', ...},"
> >
> >The keys are used for metadata might be 'recursive', 'optional', etc.
> >It might exists problem in namespace, let's use '_obj_' or '_' prefix
> >for the metadata keys.
> >
> >I used a nested dictionary to describe a DataObject, because we can
> >store the metadata and definition to different level, it's helpful
> >in parse the output by Libvirt.
> >
> > example:
> >   "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
> >
> >It's good to store _type, _name, data to same level, but the metadata
> >of items of data's value dictionary can't be appended to same level.
> >
> >    "{ '_name': 'NameInfo', '_type': 'type',
> >        'data': {
> >                  'name': 'str', '_name_optional': 'True',
> >                  'job': 'str', '_job_optional': 'True'
> >                }
> >     }"
> >
> >
> >A better solution, but I don't know if it will cause trouble for
> >Libvirt to parse the output.
> >
> >    "{'_type': 'type', '_name': 'NameInfo',
> >        'data': {  'job': {'_value': 'str', '_recursive': 'True'},
> >                   'name': {'_value': 'str', '_recursive': 'True'}
> >                },
> >        '_recursive': 'False'
> >     }"
> >
> >When we describe a DataObject (dict/list/str, one schema, extened
> >schema, schema member, etc), so I we generally use a nested dictionary
> >to describe a DataObject, it will split the metadata with original
> >data two different dictionary level, it's convenient for parse of
> >Libvirt. Here I just use a dict member as an example, actually
> >it's more complex to parse all kinds of data objects.
> >
> >>        ...
> >>        ;
> >>
> >> * Add a new type of qmp command, that returns a QString as a json literal.
> >>   query-qmp-schema is defined as this type. (This wouldn't be much code, but
> >>   may be abused in the future, I'm afraid. However we can review, limit its
> >>   use to introspection only)
> >>
> >> * And return qmp_schema_table from query-qmp-shema, which will be copied to
> >>   the wire.
> >>
> >>Feel free to disagree, it's not a perfect solution. But I really think we need
> >>to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
> >
> >
> >In the past, we didn't consider to extend and add metadata by Python, so
> >Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> >But now, we already successfully to transfer this work to Python, and
> >we can adjust to make management to be happy for the metadata and
> >format.
> >
> >The problem is that Libvirt should parse the output twice, the json
> >items are also json object string.
> >
> >Eric, Is it acceptabled?
> >
> >  Example:
> >  * as suggested by Fam to put the metadta with schema data in same
> >    dict level
> >  * process some special cases by nested dictionary
> >    (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
> >  * return a strList to Libvirt, the content of string is json object,
> >    that contains the metadata.
> >
> >{
> >    "return": [
> >        "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}",
> >        "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}",
> >        "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}",
> >        "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}",
> >        "......",
> >        "......",
> >        "......"
> >    ]
> >}
> >
> >>Fam
> >
> 
> No, I don't like this.
> 
> QAPI types are perfectly able to "describe themselves", there is no need to
> escape to JSON.
> 
> Let's just do what this series is doing, minus the unnecessary recursion.
> 

I think the interface is fine with v4, no need to change to string array. It's
just the implementation in this series shows some duplication:

                  generator in python                         parser in C
qapi-schema.json ----------------------> qapi-introspect.h ------------------> qmp result

When you look at "qmp result", it is qapi-schema.json rewritten in a formal
syntax (a real schema?). But when you look at qapi-introspect.h, that is
generated by python and parsed by C, it's a schema of the qapi-schema. So the
python code and the C code, on the arrows, are just (to a big degree) reversal
to each other, as they both implement the "schema's schema" logic: one for
generation and the other for parsing. They both write code for each type like
"command", "union", "discriminator", etc.

My question is why is this generate-and-parse necessary? Will it be reused in
the future? Can we achieve it with less duplication?

Fam
Amos Kong Jan. 28, 2014, 10:45 a.m. UTC | #8
On Mon, Jan 27, 2014 at 06:46:31PM +0800, Fam Zheng wrote:
> On Mon, 01/27 10:38, Paolo Bonzini wrote:
> > Il 27/01/2014 09:17, Amos Kong ha scritto:
> > >CC Libvirt-list
> > >
> > >Original discussion:
> > >  http://marc.info/?l=qemu-devel&m=139048842504757&w=2
> > >  [Qemu-devel] [PATCH v4 0/5] QMP full introspection
> > >
> > >On Fri, Jan 24, 2014 at 06:48:31PM +0800, Fam Zheng wrote:
> > >>On Thu, 01/23 22:46, Amos Kong wrote:
> > >>>This patch introduces a new monitor command to query QMP schema
> > >>>information, the return data is a range of schema structs, which
> > >>>contains the useful metadata to help management to check supported
> > >>>features, QMP commands detail, etc.
> > >>>
> > >>>We use qapi-introspect.py to parse all json definition in
> > >>>qapi-schema.json, and generate a range of dictionaries with metadata.
> > >>>The query command will visit the dictionaries and fill the data
> > >>>to allocated struct tree. Then QMP infrastructure will convert
> > >>>the tree to json string and return to QMP client.
> > >>>
> > >>>TODO:
> > >>>Wenchao Xia is working to convert QMP events to qapi-schema.json,
> > >>>then event can also be queried by this interface.
> > >>>
> > >>>I will introduce another command 'query-qga-schema' to query QGA
> > >>>schema information, it's easy to add this support based on this
> > >>>patch.
> > >>>
> > >>>Signed-off-by: Amos Kong <akong@redhat.com>
> > >>>---
> > >>> qapi-schema.json |  11 +++
> > >>> qmp-commands.hx  |  42 +++++++++++
> > >>> qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>> 3 files changed, 268 insertions(+)
> > >>>
> > >>>diff --git a/qapi-schema.json b/qapi-schema.json
> > >>>index c63f0ca..6033383 100644
> > >>>--- a/qapi-schema.json
> > >>>+++ b/qapi-schema.json
> > >>>@@ -4411,3 +4411,14 @@
> > >>>     'reference-type': 'String',
> > >>>     'type': 'DataObjectType',
> > >>>     'unionobj': 'DataObjectUnion' } }
> > >>>+
> > >>>+##
> > >>>+# @query-qmp-schema
> > >>>+#
> > >>>+# Query QMP schema information
> > >>>+#
> > >>>+# @returns: list of @DataObject
> > >>>+#
> > >>>+# Since: 1.8
> > >>>+##
> > >>>+{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
> > >>>diff --git a/qmp-commands.hx b/qmp-commands.hx
> > >>>index 02cc815..b83762d 100644
> > >>>--- a/qmp-commands.hx
> > >>>+++ b/qmp-commands.hx
> > >>>@@ -3291,6 +3291,48 @@ Example:
> > >>>    }
> > >>>
> > >>> 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'
> > >>>+- "returns": return data of qmp command (json-object, optional)
> > >>>+
> > >>>+Example:
> > >>>+
> > >>>+-> { "execute": "query-qmp-schema" }
> > >>>+-> { "return": [
> > >>>+     {
> > >>>+         "name": "query-name",
> > >>>+         "type": "command",
> > >>>+         "returns": {
> > >>>+             "name": "NameInfo",
> > >>>+             "type": "type",
> > >>>+             "data": [
> > >>>+                 {
> > >>>+                     "name": "name",
> > >>>+                     "optional": true,
> > >>>+                     "recursive": false,
> > >>>+                     "type": "str"
> > >>>+                 }
> > >>>+             ]
> > >>>+         }
> > >>>+     }
> > >>>+  }
> > >>>+
> > >>>+EQMP
> > >>>
> > >>>     {
> > >>>         .name       = "blockdev-add",
> > >>>diff --git a/qmp.c b/qmp.c
> > >>>index 0f46171..a64ae6d 100644
> > >>>--- a/qmp.c
> > >>>+++ b/qmp.c
> > >>>@@ -27,6 +27,8 @@
> > >>> #include "qapi/qmp/qobject.h"
> > >>> #include "qapi/qmp-input-visitor.h"
> > >>> #include "hw/boards.h"
> > >>>+#include "qapi/qmp/qjson.h"
> > >>>+#include "qapi-introspect.h"
> > >>>
> > >>> NameInfo *qmp_query_name(Error **errp)
> > >>> {
> > >>>@@ -488,6 +490,219 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> > >>>     return arch_query_cpu_definitions(errp);
> > >>> }
> > >>>
> > >>>+static strList *qobject_to_strlist(QObject *data)
> > >>>+{
> > >>>+    strList *list = NULL;
> > >>>+    strList **plist = &list;
> > >>>+    QList *qlist;
> > >>>+    const QListEntry *lent;
> > >>>+
> > >>>+    qlist = qobject_to_qlist(data);
> > >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >>>+        strList *entry = g_malloc0(sizeof(strList));
> > >>>+        entry->value = g_strdup(qobject_get_str(lent->value));
> > >>>+        *plist = entry;
> > >>>+        plist = &entry->next;
> > >>>+    }
> > >>>+
> > >>>+    return list;
> > >>>+}
> > >>>+
> > >>>+static DataObject *qobject_to_dataobj(QObject *data);
> > >>>+
> > >>>+static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> > >>>+{
> > >>>+
> > >>>+    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
> > >>>+
> > >>>+    member->type = g_malloc0(sizeof(DataObjectMemberType));
> > >>>+    if (data->type->code == QTYPE_QDICT) {
> > >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >>>+        member->type->extend = qobject_to_dataobj(data);
> > >>>+    } else {
> > >>>+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > >>>+        member->type->reference = g_strdup(qobject_get_str(data));
> > >>>+    }
> > >>>+
> > >>>+    return member;
> > >>>+}
> > >>>+
> > >>>+static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
> > >>>+{
> > >>>+    DataObjectMemberList *list = NULL;
> > >>>+    DataObjectMemberList **plist = &list;
> > >>>+    QDict *qdict = qobject_to_qdict(data);
> > >>>+    const QDictEntry *dent;
> > >>>+
> > >>>+    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
> > >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> > >>>+        entry->value = qobject_to_dataobjmem(dent->value);
> > >>>+
> > >>>+        entry->value->has_optional = true;
> > >>>+        entry->value->has_name = true;
> > >>>+        if (dent->key[0] == '*') {
> > >>>+            entry->value->optional = true;
> > >>>+            entry->value->name = g_strdup(dent->key + 1);
> > >>>+        } else {
> > >>>+            entry->value->name = g_strdup(dent->key);
> > >>>+        }
> > >>>+        *plist = entry;
> > >>>+        plist = &entry->next;
> > >>>+    }
> > >>>+
> > >>>+    return list;
> > >>>+}
> > >>>+
> > >>>+static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
> > >>>+{
> > >>>+    const QListEntry *lent;
> > >>>+    DataObjectMemberList *list = NULL;
> > >>>+    DataObjectMemberList **plist = &list;
> > >>>+    QList *qlist = qobject_to_qlist(data);
> > >>>+
> > >>>+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >>>+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
> > >>>+        entry->value = qobject_to_dataobjmem(lent->value);
> > >>>+        entry->value->has_optional = true;
> > >>>+        entry->value->has_name = true;
> > >>>+        *plist = entry;
> > >>>+        plist = &entry->next;
> > >>>+    }
> > >>>+
> > >>>+    return list;
> > >>>+}
> > >>>+
> > >>>+static DataObjectMemberList *qobject_to_memlist(QObject *data)
> > >>
> > >>This whole converting is cumbersome. You already did all the traversing through
> > >>the type jungle in python when generating this, it's not necessary to do the
> > >>similar thing again here.
> > >
> > >We can parse raw schemas and generate json string table, we can't
> > >directly return the string / qobject to monitor, C code has to convert
> > >the json to qobject, we have to revisit the qobject and convert them
> > >to DataObject/DataObjectMember/DataObject... structs.
> > >
> > >>Alternatively, I think we have a good reason to extend QMP framework as
> > >>necessary here, as we are doing "QMP introspection", which is a part of the
> > >>framework:
> > >>
> > >> * Define final output into qmp_schema_table[], no need to box it like:
> > >>
> > >>        "{'_obj_member': 'False', '_obj_type': 'enum', '_obj_name':
> > >>        'ErrorClass', '_obj_data': {'data': ...
> > >>
> > >>   just put it content of "qmp-introspection.output.txt" as a long string in
> > >>   the header,
> > >
> > >
> > >
> > >>   like you would generate in qobject_to_memlist:
> > >>
> > >>        const char *qmp_schema_table =
> > >>        "{ 'name': 'ErrorClass', 'type': 'enumeration', 'data': [...]},"
> > >>        "{ 'name': '...', ...},"
> > >
> > >The keys are used for metadata might be 'recursive', 'optional', etc.
> > >It might exists problem in namespace, let's use '_obj_' or '_' prefix
> > >for the metadata keys.
> > >
> > >I used a nested dictionary to describe a DataObject, because we can
> > >store the metadata and definition to different level, it's helpful
> > >in parse the output by Libvirt.
> > >
> > > example:
> > >   "{ 'type': 'NameInfo', 'data': {'*name': 'str', '*job': 'str'} }"
> > >
> > >It's good to store _type, _name, data to same level, but the metadata
> > >of items of data's value dictionary can't be appended to same level.
> > >
> > >    "{ '_name': 'NameInfo', '_type': 'type',
> > >        'data': {
> > >                  'name': 'str', '_name_optional': 'True',
> > >                  'job': 'str', '_job_optional': 'True'
> > >                }
> > >     }"
> > >
> > >
> > >A better solution, but I don't know if it will cause trouble for
> > >Libvirt to parse the output.
> > >
> > >    "{'_type': 'type', '_name': 'NameInfo',
> > >        'data': {  'job': {'_value': 'str', '_recursive': 'True'},
> > >                   'name': {'_value': 'str', '_recursive': 'True'}
> > >                },
> > >        '_recursive': 'False'
> > >     }"
> > >
> > >When we describe a DataObject (dict/list/str, one schema, extened
> > >schema, schema member, etc), so I we generally use a nested dictionary
> > >to describe a DataObject, it will split the metadata with original
> > >data two different dictionary level, it's convenient for parse of
> > >Libvirt. Here I just use a dict member as an example, actually
> > >it's more complex to parse all kinds of data objects.
> > >
> > >>        ...
> > >>        ;
> > >>
> > >> * Add a new type of qmp command, that returns a QString as a json literal.
> > >>   query-qmp-schema is defined as this type. (This wouldn't be much code, but
> > >>   may be abused in the future, I'm afraid. However we can review, limit its
> > >>   use to introspection only)
> > >>
> > >> * And return qmp_schema_table from query-qmp-shema, which will be copied to
> > >>   the wire.
> > >>
> > >>Feel free to disagree, it's not a perfect solution. But I really think we need
> > >>to avoid duplicating "enum", "base", "type", "union", "discriminator", ...
> > >
> > >
> > >In the past, we didn't consider to extend and add metadata by Python, so
> > >Libvirt wasn't happy to just get a raw schema(not extended, no metadata).
> > >But now, we already successfully to transfer this work to Python, and
> > >we can adjust to make management to be happy for the metadata and
> > >format.
> > >
> > >The problem is that Libvirt should parse the output twice, the json
> > >items are also json object string.
> > >
> > >Eric, Is it acceptabled?
> > >
> > >  Example:
> > >  * as suggested by Fam to put the metadta with schema data in same
> > >    dict level
> > >  * process some special cases by nested dictionary
> > >    (eg: '*tls': 'bool' ==>  'tls': {'_value': 'bool', '_optional': 'True'} )
> > >  * return a strList to Libvirt, the content of string is json object,
> > >    that contains the metadata.
> > >
> > >{
> > >    "return": [
> > >        "{'_type': 'enum', '_name': 'ErrorClass', 'data': ['GenericError', 'CommandNotFound', 'DeviceEncrypted', 'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap'], '_recursive': 'False'}",
> > >        "{'_type': 'command', '_name': 'add_client', 'data': {'tls': {'_value': 'bool', '_optional': 'True'}, 'skipauth': {'_value': 'bool', '_optional': 'True'}, 'protocol': 'str', 'fdname': 'str'}, '_recursive': 'False'}",
> > >        "{'_type': 'type', '_name': 'NameInfo', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_recursive': 'False'}",
> > >        "{'returns': {'_type': 'type', '_recursive': 'False', 'data': {'job': {'_value': 'str', '_optional': 'True'}, 'name': {'_value': 'str', '_optional': 'True'}}, '_name': 'NameInfo'}, '_name': 'query-name', '_type': 'command', '_recursive': 'False'}",
> > >        "......",
> > >        "......",
> > >        "......"
> > >    ]
> > >}
> > >
> > >>Fam
> > >
> > 
> > No, I don't like this.
> > 
> > QAPI types are perfectly able to "describe themselves", there is no need to
> > escape to JSON.
> > 
> > Let's just do what this series is doing, minus the unnecessary recursion.
> > 
> 
> I think the interface is fine with v4, no need to change to string array. It's
> just the implementation in this series shows some duplication:
 
The V4 output is very close to original conclusion about DataObject/metadata.

>                   generator in python                         parser in C
> qapi-schema.json ----------------------> qapi-introspect.h ------------------> qmp result
> 
> When you look at "qmp result", it is qapi-schema.json rewritten in a formal
> syntax (a real schema?). But when you look at qapi-introspect.h, that is
> generated by python and parsed by C, it's a schema of the qapi-schema. So the
> python code and the C code, on the arrows, are just (to a big degree) reversal
> to each other, as they both implement the "schema's schema" logic: one for
> generation and the other for parsing. They both write code for each type like
> "command", "union", "discriminator", etc.

Right, we can't avoid the duplicate if we want to connect multiple
interfaces (Python, QMP, QAPI, JSON).
 
> My question is why is this generate-and-parse necessary?

It's request of Libvirt, actually we can directly return the raw
schema to Libvirt without extending/parsing, then Libvirt parse
by itself.

> Will it be reused in the future?

It seems not.

> Can we achieve it with less duplication?
> 
> Fam

Let's see the feedback of Eric.

Thanks, Amos
Paolo Bonzini Jan. 28, 2014, 11:14 a.m. UTC | #9
Il 28/01/2014 11:45, Amos Kong ha scritto:
> > My question is why is this generate-and-parse necessary?
>
> It's request of Libvirt, actually we can directly return the raw
> schema to Libvirt without extending/parsing, then Libvirt parse
> by itself.
>
> > Can we achieve it with less duplication?
>
> Let's see the feedback of Eric.

Eric's feedback is certainly useful, but I think we need to look at it 
from the QEMU perspective more than the libvirt perspective.

Passing the raw schema and letting libvirt parse it is a Really Bad idea 
from the QEMU perspective, in my opinion, even if it means a little more 
work now and even if libvirt is willing to add the parser.

First and foremost, the current "pseudo-JSON" encoding of the schema is 
nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax 
definitely shouldn't percolate to the QAPI documentation.  Using normal 
QAPI structs means that the normal tool for documentation 
(qapi-schema.json doc comments) applies just as well to QAPI schema 
introspection

Second, if one day we were to change the schema representation from 
"pseudo-JSON" to something else, we would have to carry a "pseudo-JSON" 
serializer for backwards compatibility.  Building QAPI structs and 
relying on the normal formatting machinery is very different from 
putting together strings manually.

The schema must be emitted as JSON data, not as a string.  I'm not 
willing to compromise on this point. :)

Paolo
Eric Blake Jan. 28, 2014, 1:58 p.m. UTC | #10
On 01/28/2014 04:14 AM, Paolo Bonzini wrote:

>> Let's see the feedback of Eric.
> 
> Eric's feedback is certainly useful, but I think we need to look at it
> from the QEMU perspective more than the libvirt perspective.
> 
> Passing the raw schema and letting libvirt parse it is a Really Bad idea
> from the QEMU perspective, in my opinion, even if it means a little more
> work now and even if libvirt is willing to add the parser.

Libvirt wants to parse formal qapi, not pseudo-JSON.  I still have on my
to-do list to read the v4 schema and make sure that libvirt can live
with it.

> 
> First and foremost, the current "pseudo-JSON" encoding of the schema is
> nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax
> definitely shouldn't percolate to the QAPI documentation.  Using normal
> QAPI structs means that the normal tool for documentation
> (qapi-schema.json doc comments) applies just as well to QAPI schema
> introspection

Agreed - I definitely want the output of the query command to be fully
described by qapi.  Which means we DO have to convert from the
pseudo-JSON of the qapi file into the final formal qapi format.  But the
conversion is known at code generation time, so you should do it as part
of your python code generator, and not repeat the conversion at runtime
in the C code.  That is, the C code should have everything already split
out into the data structures it needs to just output the qapi structure
as documented for the formal qapi definition.

> 
> Second, if one day we were to change the schema representation from
> "pseudo-JSON" to something else, we would have to carry a "pseudo-JSON"
> serializer for backwards compatibility.  Building QAPI structs and
> relying on the normal formatting machinery is very different from
> putting together strings manually.
> 
> The schema must be emitted as JSON data, not as a string.  I'm not
> willing to compromise on this point. :)

Nor am I.  The output MUST be formal JSON, described by the qapi
(whether we change the syntax of qapi-schema.json and tweak the .py code
generators in the future should not matter, the output of the QMP
command will be the same formal JSON).
Fam Zheng Jan. 29, 2014, 8:12 a.m. UTC | #11
On Tue, 01/28 06:58, Eric Blake wrote:
> On 01/28/2014 04:14 AM, Paolo Bonzini wrote:
> 
> >> Let's see the feedback of Eric.
> > 
> > Eric's feedback is certainly useful, but I think we need to look at it
> > from the QEMU perspective more than the libvirt perspective.
> > 
> > Passing the raw schema and letting libvirt parse it is a Really Bad idea
> > from the QEMU perspective, in my opinion, even if it means a little more
> > work now and even if libvirt is willing to add the parser.
> 
> Libvirt wants to parse formal qapi, not pseudo-JSON.  I still have on my
> to-do list to read the v4 schema and make sure that libvirt can live
> with it.
> 
> > 
> > First and foremost, the current "pseudo-JSON" encoding of the schema is
> > nothing but a QEMU implementation detail.  The "pseudo-JSON" syntax
> > definitely shouldn't percolate to the QAPI documentation.  Using normal
> > QAPI structs means that the normal tool for documentation
> > (qapi-schema.json doc comments) applies just as well to QAPI schema
> > introspection
> 
> Agreed - I definitely want the output of the query command to be fully
> described by qapi.  Which means we DO have to convert from the
> pseudo-JSON of the qapi file into the final formal qapi format.  But the
> conversion is known at code generation time, so you should do it as part
> of your python code generator, and not repeat the conversion at runtime
> in the C code.  That is, the C code should have everything already split
> out into the data structures it needs to just output the qapi structure
> as documented for the formal qapi definition.
> 

Yes, that's exactly what I mean.

If we use python to generate code for qobject_to_dataobj() or even generate
object_to_$type() for each qapi type, the C code needed will be minimal anyway.

Thanks,
Fam
Eric Blake Feb. 4, 2014, 12:33 a.m. UTC | #12
On 01/23/2014 07:46 AM, Amos Kong wrote:
> This patch introduces a new monitor command to query QMP schema
> information, the return data is a range of schema structs, which
> contains the useful metadata to help management to check supported
> features, QMP commands detail, etc.
> 
> We use qapi-introspect.py to parse all json definition in
> qapi-schema.json, and generate a range of dictionaries with metadata.
> The query command will visit the dictionaries and fill the data
> to allocated struct tree. Then QMP infrastructure will convert
> the tree to json string and return to QMP client.
> 
> TODO:

Do we still want this TODO in the commit message?  It would be nice to
get both of these features (introspection, and events described in qapi)
in for 2.0; I'm trying to spend some review time to help get us there.

> Wenchao Xia is working to convert QMP events to qapi-schema.json,
> then event can also be queried by this interface.
> 
> I will introduce another command 'query-qga-schema' to query QGA
> schema information, it's easy to add this support based on this
> patch.

Yes, we want that too :)  Have you started that patch, or are you trying
to get more feedback on this patch to make sure you're on the right track?

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qapi-schema.json |  11 +++
>  qmp-commands.hx  |  42 +++++++++++
>  qmp.c            | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 268 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c63f0ca..6033383 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4411,3 +4411,14 @@
>      'reference-type': 'String',
>      'type': 'DataObjectType',
>      'unionobj': 'DataObjectUnion' } }
> +
> +##
> +# @query-qmp-schema
> +#
> +# Query QMP schema information
> +#
> +# @returns: list of @DataObject
> +#
> +# Since: 1.8

2.0

And here's where an optional bool on whether to expand may be
worthwhile, as well as an optional argument allowing callers to filter data.

> +##
> +{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }

> +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'

s/comand/command/

> +++ b/qmp.c
> +static DataObjectMember *qobject_to_dataobjmem(QObject *data)
> +{
> +
> +    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));

The blank line here looks unusual.

I didn't look at the code very closely; I want to look at the generated
.h file first.
diff mbox

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index c63f0ca..6033383 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4411,3 +4411,14 @@ 
     'reference-type': 'String',
     'type': 'DataObjectType',
     'unionobj': 'DataObjectUnion' } }
+
+##
+# @query-qmp-schema
+#
+# Query QMP schema information
+#
+# @returns: list of @DataObject
+#
+# Since: 1.8
+##
+{ 'command': 'query-qmp-schema', 'returns': ['DataObject'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 02cc815..b83762d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3291,6 +3291,48 @@  Example:
    }
 
 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'
+- "returns": return data of qmp command (json-object, optional)
+
+Example:
+
+-> { "execute": "query-qmp-schema" }
+-> { "return": [
+     {
+         "name": "query-name",
+         "type": "command",
+         "returns": {
+             "name": "NameInfo",
+             "type": "type",
+             "data": [
+                 {
+                     "name": "name",
+                     "optional": true,
+                     "recursive": false,
+                     "type": "str"
+                 }
+             ]
+         }
+     }
+  }
+
+EQMP
 
     {
         .name       = "blockdev-add",
diff --git a/qmp.c b/qmp.c
index 0f46171..a64ae6d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -27,6 +27,8 @@ 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp-input-visitor.h"
 #include "hw/boards.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi-introspect.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -488,6 +490,219 @@  CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
     return arch_query_cpu_definitions(errp);
 }
 
+static strList *qobject_to_strlist(QObject *data)
+{
+    strList *list = NULL;
+    strList **plist = &list;
+    QList *qlist;
+    const QListEntry *lent;
+
+    qlist = qobject_to_qlist(data);
+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
+        strList *entry = g_malloc0(sizeof(strList));
+        entry->value = g_strdup(qobject_get_str(lent->value));
+        *plist = entry;
+        plist = &entry->next;
+    }
+
+    return list;
+}
+
+static DataObject *qobject_to_dataobj(QObject *data);
+
+static DataObjectMember *qobject_to_dataobjmem(QObject *data)
+{
+
+    DataObjectMember *member = g_malloc0(sizeof(DataObjectMember));
+
+    member->type = g_malloc0(sizeof(DataObjectMemberType));
+    if (data->type->code == QTYPE_QDICT) {
+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
+        member->type->extend = qobject_to_dataobj(data);
+    } else {
+        member->type->kind = DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
+        member->type->reference = g_strdup(qobject_get_str(data));
+    }
+
+    return member;
+}
+
+static DataObjectMemberList *qobject_to_dict_memlist(QObject *data)
+{
+    DataObjectMemberList *list = NULL;
+    DataObjectMemberList **plist = &list;
+    QDict *qdict = qobject_to_qdict(data);
+    const QDictEntry *dent;
+
+    for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict, dent)) {
+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
+        entry->value = qobject_to_dataobjmem(dent->value);
+
+        entry->value->has_optional = true;
+        entry->value->has_name = true;
+        if (dent->key[0] == '*') {
+            entry->value->optional = true;
+            entry->value->name = g_strdup(dent->key + 1);
+        } else {
+            entry->value->name = g_strdup(dent->key);
+        }
+        *plist = entry;
+        plist = &entry->next;
+    }
+
+    return list;
+}
+
+static DataObjectMemberList *qobject_to_list_memlist(QObject *data)
+{
+    const QListEntry *lent;
+    DataObjectMemberList *list = NULL;
+    DataObjectMemberList **plist = &list;
+    QList *qlist = qobject_to_qlist(data);
+
+    for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
+        DataObjectMemberList *entry = g_malloc0(sizeof(DataObjectMemberList));
+        entry->value = qobject_to_dataobjmem(lent->value);
+        entry->value->has_optional = true;
+        entry->value->has_name = true;
+        *plist = entry;
+        plist = &entry->next;
+    }
+
+    return list;
+}
+
+static DataObjectMemberList *qobject_to_memlist(QObject *data)
+{
+    DataObjectMemberList *list = NULL;
+    QDict *qdict = qobject_to_qdict(data);
+    QObject *subdata = qdict_get(qdict, "_obj_data");
+
+    list = NULL;
+    if (subdata->type->code == QTYPE_QDICT) {
+        list = qobject_to_dict_memlist(subdata);
+    } else if (subdata->type->code == QTYPE_QLIST) {
+        list = qobject_to_list_memlist(subdata);
+    }
+
+    return list;
+}
+
+static DataObject *qobject_to_dataobj(QObject *data)
+{
+    QObject *subdata;
+    QDict *qdict;
+    const char *obj_type, *obj_recursive;
+    DataObject *obj = g_malloc0(sizeof(DataObject));
+
+    if (data->type->code == QTYPE_QSTRING) {
+        obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
+        obj->reference_type = g_malloc0(sizeof(String));
+        obj->reference_type->str = g_strdup(qobject_get_str(data));
+        return obj;
+    }
+
+    qdict = qobject_to_qdict(data);
+    assert(qdict != NULL);
+
+    obj_type = qobject_get_str(qdict_get(qdict, "_obj_type"));
+    obj_recursive = qobject_get_str(qdict_get(qdict, "_obj_recursive"));
+    if (!strcmp(obj_recursive, "True")) {
+        obj->has_recursive = true;
+        obj->recursive = true;
+    }
+
+    obj->has_name = true;
+    obj->name = g_strdup(qobject_get_str(qdict_get(qdict, "_obj_name")));
+
+    subdata = qdict_get(qdict, "_obj_data");
+    qdict = qobject_to_qdict(subdata);
+
+    if (!strcmp(obj_type, "command")) {
+        obj->kind = DATA_OBJECT_KIND_COMMAND;
+        obj->command = g_malloc0(sizeof(DataObjectCommand));
+        subdata = qdict_get(qobject_to_qdict(subdata), "data");
+
+        if (subdata && subdata->type->code == QTYPE_QDICT) {
+            obj->command->has_data = true;
+            obj->command->data = qobject_to_memlist(subdata);
+        } else if (subdata && subdata->type->code == QTYPE_QLIST) {
+            abort();
+        }
+
+        subdata = qdict_get(qdict, "returns");
+        if (subdata) {
+            obj->command->has_returns = true;
+            obj->command->returns = qobject_to_dataobj(subdata);
+        }
+
+        subdata = qdict_get(qdict, "gen");
+        if (subdata && subdata->type->code == QTYPE_QSTRING) {
+            obj->command->has_gen = true;
+            if (!strcmp(qobject_get_str(subdata), "no")) {
+                obj->command->gen = false;
+            } else {
+                obj->command->gen = true;
+            }
+        }
+    } else if (!strcmp(obj_type, "union")) {
+        obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
+        obj->unionobj = g_malloc0(sizeof(DataObjectUnion));
+        subdata = qdict_get(qdict, "data");
+        obj->unionobj->data = qobject_to_memlist(subdata);
+
+        subdata = qdict_get(qdict, "base");
+        if (subdata) {
+            obj->unionobj->has_base = true;
+            obj->unionobj->base = qobject_to_dataobj(subdata);
+        }
+
+        subdata = qdict_get(qdict, "discriminator");
+        if (subdata) {
+            obj->unionobj->has_discriminator = true;
+            obj->unionobj->discriminator = qobject_to_dataobj(subdata);
+        }
+    } else if (!strcmp(obj_type, "type")) {
+        obj->kind = DATA_OBJECT_KIND_TYPE;
+        obj->type = g_malloc0(sizeof(DataObjectType));
+        subdata = qdict_get(qdict, "data");
+        if (subdata) {
+            obj->type->data = qobject_to_memlist(subdata);
+        }
+     } else if (!strcmp(obj_type, "enum")) {
+        obj->kind = DATA_OBJECT_KIND_ENUMERATION;
+        obj->enumeration = g_malloc0(sizeof(DataObjectEnumeration));
+        subdata = qdict_get(qdict, "data");
+        obj->enumeration->data = qobject_to_strlist(subdata);
+    } else {
+        obj->has_name = false;
+        obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
+        obj->anonymous_struct = g_malloc0(sizeof(DataObjectAnonymousStruct));
+        obj->anonymous_struct->data = qobject_to_memlist(data);
+    }
+
+    return obj;
+}
+
+DataObjectList *qmp_query_qmp_schema(Error **errp)
+{
+    DataObjectList *list = NULL;
+    DataObjectList **plist = &list;
+    QObject *data;
+    int i;
+
+    for (i = 0; qmp_schema_table[i]; i++) {
+        data = qobject_from_json(qmp_schema_table[i]);
+        assert(data != NULL);
+        DataObjectList *entry = g_malloc0(sizeof(DataObjectList));
+        entry->value = qobject_to_dataobj(data);
+        *plist = entry;
+        plist = &entry->next;
+    }
+
+    return list;
+}
+
 void qmp_add_client(const char *protocol, const char *fdname,
                     bool has_skipauth, bool skipauth, bool has_tls, bool tls,
                     Error **errp)