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

[Amos Kong]

On Wed, Jul 17, 2013 at 04:36:06PM -0400, Luiz Capitulino wrote:
> On Tue, 16 Jul 2013 18:37:42 +0800
> Amos Kong <address@hidden> wrote:
> 
> > Introduces new monitor command to query QMP schema information,
> > the return data is a dynamical and nested dict/list, it contains
> > the useful metadata to help management to check feature support,
> > QMP commands detail, etc.
> > 
> > I added a document for QMP introspection support.
> > (docs/qmp-full-introspection.txt)
> > 
> > We need to parse all commands json definition, and generated a
> > dynamical tree, QMP infrastructure will convert the tree to
> > json string and return to QMP client.
> > 
> > So here I defined a 'DataObject' type in qapi-schema.json,
> > it's used to describe the dynamical dictionary/list/string.
> 
> I skimmed over the patch and made some comments, but my impression
> is that this is getting too complex... Why did we move from letting
> mngt query type by type (last version) to this version which returns
> all commands and their input types? Does this satisfy libvirt needs?
 

> > diff --git a/qmp.c b/qmp.c
> > index 4c149b3..3ace3a6 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -25,6 +25,8 @@
> >  #include "sysemu/blockdev.h"
> >  #include "qom/qom-qobject.h"
> >  #include "hw/boards.h"
> > +#include "qmp-schema.h"
> > +#include "qapi/qmp/qjson.h"
> >  
> >  NameInfo *qmp_query_name(Error **errp)
> >  {
> > @@ -486,6 +488,311 @@ CpuDefinitionInfoList 
> > *qmp_query_cpu_definitions(Error **errp)
> >      return arch_query_cpu_definitions(errp);
> >  }
> >  
> > +/*
> > + * Use a string to record the visit path, type index of each node
> > + * will be saved to the string, indexes are split by ':'.
> > + */
> > +static char visit_path_str[1024];
> 
> visit path? I don't get this.

The return data is a dynamic tree. When we found defined type, we need to 
extend it.
But some defined type is used in its child's node. So I use a visit_path_str to
record the type index in one node. We don't extend one type, if it's already
been extened in parent's node.

{'type': 'a', 'data': ['a', 'b', 'c']}
{'command': 'cmd', 'data': 'a'} 

'a' is a defined type, we will extended it. But we will not extend 'a'
in data list of type 'a'.

I will add the explain to the doc.
    
> > +
> > +/* push the type index to visit_path_str */
> > +static void push_id(int id)
> > +{
> > +    char *end = strrchr(visit_path_str, ':');
> > +    char type_idx[256];
> > +    int num;
> > +
> > +    num = sprintf(type_idx, "%d:", id);
> > +
> > +    if (end) {
> > +        /* avoid overflow */
> > +        assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> > +        sprintf(end + 1, "%d:", id);
> > +    } else {
> > +        sprintf(visit_path_str, "%d:", id);
> > +    }
> > +}
> > +
> > +/* pop the type index from visit_path_str */
> > +static void pop_id(void)
> > +{
> > +    char *p = strrchr(visit_path_str, ':');
> > +
> > +    assert(p != NULL);
> > +    *p = '\0';
> > +    p = strrchr(visit_path_str, ':');
> > +    if (p) {
> > +        *(p + 1) = '\0';
> > +    } else {
> > +        visit_path_str[0] = '\0';
> > +    }
> > +}


> > +SchemaEntryList *qmp_query_qmp_schema(Error **errp)
> > +{
> > +    SchemaEntryList *list, *last_entry, *entry;
> > +    SchemaEntry *info;
> > +    DataObjectList *obj_entry;
> > +    DataObject *obj_info;
> > +    QObject *data;
> > +    QDict *qdict;
> > +    int i;
> > +
> > +    list = NULL;
> > +    for (i = 0; qmp_schema_table[i]; i++) {
> > +        data = qobject_from_json(qmp_schema_table[i]);
> > +        assert(data != NULL);
> > +
> > +        qdict = qobject_to_qdict(data);
> > +        assert(qdict != NULL);
> > +
> > +        if (qdict_get(qdict, "command")) {
> > +            info = g_malloc0(sizeof(*info));
> > +            info->type = SCHEMA_META_TYPE_COMMAND;
> > +            info->name = strdup(qdict_get_str(qdict, "command"));
> 
> s/strdup/g_strdup
> 
> > +        } else {
> > +            continue;
> > +        }
> 
> You return only commands. That is, types are returned as part of the
> command input.

Right.

> ErrorClass can't be queried then? I'm not judging, only
> observing.

We can return the 'type', 'enum', 'union', 'etc' if libvirt needs them.

> Eric, this is good enough for libvirt?
> 
> Btw, you're leaking data on the else clause.
> 
> > +
> > +        memset(visit_path_str, 0, sizeof(visit_path_str));
> > +        data = qdict_get(qdict, "data");
> > +        if (data) {
> > +            info->has_data = true;
> > +            if (data->type->code == QTYPE_QLIST) {
> > +                info->data = visit_qobj_list(data);
> > +            } else if (data->type->code == QTYPE_QDICT) {
> > +                info->data = visit_qobj_dict(data);
> > +            } else if (data->type->code == QTYPE_QSTRING) {
> > +                info->data = 
> > extend_type(qstring_get_str(qobject_to_qstring(data)));
> > +                if (!info->data) {
> > +                    obj_info = g_malloc0(sizeof(*obj_info));
> > +                    obj_entry = g_malloc0(sizeof(*obj_entry));
> > +                    obj_entry->value = obj_info;
> > +                    obj_info->has_type = true;
> > +                    obj_info->type = g_strdup(qdict_get_str(qdict, 
> > "data"));
> > +                    info->data = obj_entry;
> > +                }
> > +            } else {
> > +                abort();
> 
> Pleae, explain why you're aborting here.

{ 'command': 'query-qmp-schema', 'data': XXXX }

the type of XXXX can only be list, dictionary or buildin-type.
XXXX is the value in define dictionary.
 
> > +            }
> > +        }

-- 
                        Amos.