[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] qmp: full introspection support for QMP
|
From: |
Luiz Capitulino |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] qmp: full introspection support for QMP |
|
Date: |
Wed, 22 Jan 2014 13:18:45 -0500 |
On Thu, 9 Jan 2014 17:49:56 +0800
Amos Kong <address@hidden> wrote:
> On Mon, Jan 06, 2014 at 05:37:19PM +0800, Fam Zheng wrote:
> > On 2014年01月05日 20:02, 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.
> > >
> > >It parses all json definition in qapi-schema.json, and generate a
> > >dynamic struct tree, QMP infrastructure will convert the tree to
> > >json string and return to QMP client.
> > >
> > >I defined a 'DataObject' union in qapi-schema.json, it's used to
> > >describe the dynamic data struct.
> > >
> > >I also added a document about QMP full introspection support
> > >(docs/qmp-full-introspection.txt), it helps to use the new interface
> > >and understand the abstract method in describing the dynamic struct.
> > >
> > >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 <address@hidden>
> > >---
>
> Hi Fam,
>
> I'm very happy to get your comments!
>
> > I have a few comments on the current implementation below, there are
> > a few things to improve. However I agree to what Eric suggested in
> > reply to V2: it may be better to generate most of the response data
> > in python code at compile time and simplify the logic in C. Because
> > this implementation is slow and it is unnecessary runtime
> > computation. It also duplicates much of existing qapi.py logic (data
> > types and other semantics parsing).
>
> The implemented code of QMP command will generate defined struct,
> the memory is allocated in run time, it will be auto released by
> QMP infrastructure.
>
> We can implement the parse/extend work by Python and generate
> a result in a head file, that can be directly & easyly used C code.
> Maybe we can generate a nested & complex struct definition in the
> head file, and use less g_malloc() to generate return struct.
> I will try, thanks.
Which means this patch will completely change, right? In this case
I'm skipping review.
>
> > > docs/qmp-full-introspection.txt | 97 ++++++++++
> > > qapi-schema.json | 150 ++++++++++++++++
> > > qmp-commands.hx | 43 ++++-
> > > qmp.c | 382
> > > ++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 671 insertions(+), 1 deletion(-)
> > > create mode 100644 docs/qmp-full-introspection.txt
> > >
> > >diff --git a/docs/qmp-full-introspection.txt
> > >b/docs/qmp-full-introspection.txt
> > >new file mode 100644
> > >index 0000000..1617df7
> > >--- /dev/null
> > >+++ b/docs/qmp-full-introspection.txt
> > >@@ -0,0 +1,97 @@
> > >+= Full introspection support for QMP =
> > >+
> > >+
> > >+== Purpose ==
> > >+
> > >+Add a new monitor command for management to query QMP schema
> > >+information, it returns a range of schema structs, which contain the
> > >+useful metadata to help management to check supported features, QMP
> > >+commands detail, etc.
> > >+
> > >+== Usage ==
> > >+
> > >+Json schema:
> > >+ { 'type': 'NameInfo', 'data': {'*name': 'str'} }
> > >+ { 'command': 'query-name', 'returns': 'NameInfo' }
> > >+
> > >+Execute QMP command:
> > >+
> > >+ { "execute": "query-qmp-schema" }
> > >+
> > >+Returns:
> > >+
> > >+ { "return": [
> > >+ {
> > >+ "name": "query-name",
> > >+ "type": "command",
> > >+ "returns": {
> > >+ "name": "NameInfo",
> > >+ "type": "type",
> > >+ "data": [
> > >+ {
> > >+ "name": "name",
> > >+ "optional": true,
> > >+ "recursive": false,
> > >+ "type": "str"
> > >+ }
> > >+ ]
> > >+ }
> > >+ },
> > >+ ...
> > >+ }
> > >+
> > >+The whole schema information will be returned in one go, it contains
> > >+all the schema entries. It doesn't support to be filtered by type
> > >+or name. Currently it takes about 5 seconds to return about 1.5M string.
> > >+
> > >+== 'DataObject' union ==
> > >+
> > >+{ 'union': 'DataObject',
> > >+ 'base': 'DataObjectBase',
> > >+ 'discriminator': 'type',
> > >+ 'data': {
> > >+ 'anonymous-struct': 'DataObjectAnonymousStruct',
> > >+ 'command': 'DataObjectCommand',
> > >+ 'enumeration': 'DataObjectEnumeration',
> > >+ 'reference-type': 'String',
> > >+ 'type': 'DataObjectType',
> > >+ 'unionobj': 'DataObjectUnion' } }
> > >+
> > >+Currently we have schema difinitions for type, command, enumeration,
> > >+union. Some arbitrary structs (dictionary, list or string) and native
> > >+types are also used in the body of definitions.
> > >+
> > >+Here we use "DataObject" union to abstract all above schema. We want
> > >+to provide more useful metadata, and used some enum/unions to indicate
> > >+the dynamic type. In the output, some simple data is processed too
> > >+unwieldy. In another side, some complex data is described clearly.
> > >+It's also caused by some limitation of QAPI infrastructure.
> > >+
> > >+So we define 'DataObject' to be an union, it always has an object name
> > >+except anonymous struct.
> > >+
> > >+'command', 'enumeration', 'type', 'unionobj' are common schema type,
> > >+'union' is a build-in type, so I used unionobj here.
> > >+
> > >+'reference-type' will be used to describe native types and unextended
> > >+types.
> > >+
> > >+'anonymous-struct' will be used to describe arbitrary structs
> > >+(dictionary, list or string).
> > >+
> > >+== Avoid dead loop in recursive extending ==
> > >+
> > >+We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
> > >+that uses themself in their own define data directly or indirectly,
> > >+we will not repeatedly extend them to avoid dead loop.
> > >+
> > >+We use a string to record the visit path, type index of each node
> > >+will be saved to the string, indexes are split by ':'.
> > >+
> > >+Push index to visit_path_str before extending, and pop index from
> > >+visit_path_str after extending.
> > >+
> > >+If the type was already extended in parent node, we don't extend it
> > >+again to avoid dead loop.
> > >+
> > >+'recursive' indicates if the type is extended or not.
> > >diff --git a/qapi-schema.json b/qapi-schema.json
> > >index c3c939c..db500ab 100644
> > >--- a/qapi-schema.json
> > >+++ b/qapi-schema.json
> > >@@ -4235,3 +4235,153 @@
> > > # Since: 1.7
> > > ##
> > > { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
> > >+
> > >+##
> > >+# @DataObjectBase
> > >+#
> > >+# Base of @DataObject
> > >+#
> > >+# @name: #optional @DataObject name
> > >+# @type: @DataObject type
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectBase',
> > >+ 'data': { '*name': 'str', 'type': 'str' } }
> > >+
> > >+##
> > >+# @DataObjectMemberType
> > >+#
> > >+# Type of @DabaObjectMember
> > >+#
> > >+# @reference: reference string
> > >+# @anonymous: arbitrary struct
> > >+# @extend: the @DataObjectMember
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'union': 'DataObjectMemberType',
> > >+ 'discriminator': {},
> > >+ 'data': { 'reference': 'str',
> > >+ 'anonymous': 'DataObject',
> > >+ 'extend': 'DataObject' } }
> > >+
> > >+##
> > >+# @DataObjectMember
> > >+#
> > >+# General member of @DataObject
> > >+#
> > >+# @type: type of @DataObjectMember
> > >+# @name: #optional name
> > >+# @optional: #optional key to indicate if the @DataObjectMember is
> > >optional
> > >+# @recursive: #optional key to indicate if it's defined recursively
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectMember',
> > >+ 'data': { 'type': 'DataObjectMemberType', '*name': 'str',
> > >+ '*optional': 'bool', '*recursive': 'bool' } }
> > >+
> > >+##
> > >+# @DataObjectAnonymousStruct
> > >+#
> > >+# Arbitrary struct, it can be dictionary, list or string
> > >+#
> > >+# @data: content of arbitrary struct
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectAnonymousStruct',
> > >+ 'data': { 'data': [ 'DataObjectMember' ] } }
> > >+
> > >+##
> > >+# @DataObjectCommand
> > >+#
> > >+# QMP Command schema
> > >+#
> > >+# @data: QMP command content
> > >+# @returns: returns of executing command
> > >+# @gen: a key to suppress code generation
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectCommand',
> > >+ 'data': { '*data': [ 'DataObjectMember' ],
> > >+ '*returns': 'DataObject',
> > >+ '*gen': 'bool' } }
> > >+
> > >+##
> > >+# @DataObjectEnumeration
> > >+#
> > >+# Enumeration schema
> > >+#
> > >+# @data: enumeration content, it's a string list
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectEnumeration',
> > >+ 'data': { 'data': [ 'str' ] } }
> > >+
> > >+##
> > >+# @DataObjectType
> > >+#
> > >+# Type schema
> > >+#
> > >+# @data: type content
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectType',
> > >+ 'data': { 'data': [ 'DataObjectMember' ] } }
> > >+
> > >+##
> > >+# @DataObjectUnion
> > >+#
> > >+# Union schema
> > >+#
> > >+# @data: union content
> > >+# @base: union base
> > >+# @discriminator: union discriminator
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'type': 'DataObjectUnion',
> > >+ 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str',
> > >+ '*discriminator': 'str' } }
> > >+
> > >+##
> > >+# @DataObject
> > >+#
> > >+# Dynamic data struct, it can be command, enumeration, type, union,
> > >arbitrary
> > >+# struct or native type.
> > >+#
> > >+# @anonymous-struct: arbitrary struct, it can be dictionary, list or
> > >string
> > >+# @command: QMP command schema
> > >+# @enumeration: enumeration schema
> > >+# @reference-type: native type or unextended type
> > >+# @type: type schema, it will be extended
> > >+# @unionobj: union schema
> > >+#
> > >+# Since: 1.8
> > >+##
> > >+{ 'union': 'DataObject',
> > >+ 'base': 'DataObjectBase',
> > >+ 'discriminator': 'type',
> > >+ 'data': {
> > >+ 'anonymous-struct': 'DataObjectAnonymousStruct',
> > >+ 'command': 'DataObjectCommand',
> > >+ 'enumeration': 'DataObjectEnumeration',
> > >+ '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 fba15cd..cf9c4aa 100644
> > >--- a/qmp-commands.hx
> > >+++ b/qmp-commands.hx
> > >@@ -3237,7 +3237,48 @@ Example:
> > > "broadcast-allowed": false
> > > }
> > > ]
> > >- }
> > >+
> > >+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
> > >
> > >diff --git a/qmp.c b/qmp.c
> > >index 1d7a04d..e9bba06 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,386 @@ CpuDefinitionInfoList
> > >*qmp_query_cpu_definitions(Error **errp)
> > > return arch_query_cpu_definitions(errp);
> > > }
> > >
> > >+/*
> > >+ * use a passed string to record the visit path, schema index of
> > >+ * each extended node will be appended to the string, indexes are
> > >+ * split by ':'
> > >+ */
> > >+static char visit_path_str[1024];
> >
> > Is it safe to use a global variable here? Is there any race
> > condition (i.e. multiple monitor commands run parallel)?
>
> This command is only executed once after installing/updating qemu
> for Libvirt. But the problem you pointed is existed. I will allocate
> /free dynamic memory for each query instance.
>
> > IIUC this serves as a queue of number? Why not use array of int?
>
> Great! It's easier to understand and update.
>
> > >+
> > >+/* push the type index into visit_path_str */
> > >+static void push_id(int id)
> > >+{
> > >+ char *end = strrchr(visit_path_str, ':');
> > >+ char type_idx[256];
> > >+ int num;
> > >+
> > >+ num = sprintf(type_idx, "%d:", id);
> > >+
> > >+ if (end) {
> > >+ /* avoid overflow */
> > >+ assert(end - visit_path_str + 1 + num < sizeof(visit_path_str));
> > >+ sprintf(end + 1, "%d:", id);
> > >+ } else {
> > >+ sprintf(visit_path_str, "%d:", id);
> > >+ }
> > >+}
> > >+
> > >+/* pop the type index from visit_path_str */
> > >+static void pop_id(void)
> > >+{
> > >+ char *p = strrchr(visit_path_str, ':');
> > >+
> > >+ assert(p != NULL);
> > >+ *p = '\0';
> > >+ p = strrchr(visit_path_str, ':');
> > >+ if (p) {
> > >+ *(p + 1) = '\0';
> > >+ } else {
> > >+ visit_path_str[0] = '\0';
> > >+ }
> > >+}
> > >+
> > >+static const char *qstring_copy_str(QObject *data)
> >
> > The string is not copied, it's misleading. And I think you could
> > just use qstring_get_str in the caller or define a new qstring_ util
> > function in qobject/qstring.c.
>
> OK.
>
> >
> > >+{
> > >+ QString *qstr;
> > >+
> > >+ if (!data) {
> > >+ return NULL;
> > >+ }
> > >+ qstr = qobject_to_qstring(data);
> > >+ if (qstr) {
> > >+ return qstring_get_str(qstr);
> > >+ } else {
> > >+ return NULL;
> > >+ }
> > >+}
> > >+
> > >+static QObject *get_definition(const char *str, bool update_path)
> > >+{
> > >+ QObject *data, *value;
> > >+ QDict *qdict;
> > >+ int i;
> > >+
> > >+ if (!strcmp(str, "str") || !strcmp(str, "int") ||
> > >+ !strcmp(str, "number") || !strcmp(str, "bool") ||
> > >+ !strcmp(str, "int8") || !strcmp(str, "int16") ||
> > >+ !strcmp(str, "int32") || !strcmp(str, "int64") ||
> > >+ !strcmp(str, "uint8") || !strcmp(str, "uint16") ||
> > >+ !strcmp(str, "uint32") || !strcmp(str, "uint64") ||
> > >+ !strcmp(str, "visitor") || !strcmp(str, "**") ||
> > >+ !strcmp(str, "size")) {
> > >+ return NULL;
> > >+ }
> > >+
> > >+ for (i = 0; qmp_schema_table[i]; i++) {
> > >+ data = qobject_from_json(qmp_schema_table[i]);
> > >+ qdict = qobject_to_qdict(data);
> > >+ assert(qdict != NULL);
> > >+
> > >+ if (qdict_get(qdict, "enum")) {
> > >+ value = qdict_get(qdict, "enum");
> > >+ } else if (qdict_get(qdict, "type")) {
> > >+ value = qdict_get(qdict, "type");
> > >+ } else if (qdict_get(qdict, "union")) {
> > >+ value = qdict_get(qdict, "union");
> > >+ } else {
> > >+ continue;
> > >+ }
> > >+
> > >+ if (!strcmp(str, qstring_copy_str(value)) && update_path) {
> >
> > Simply:
> >
> > if (strcmp(str, qstring_copy_str(value)) {
> > continue;
> > }
> >
> > Then avoid another check for this.
> >
> > >+ char *start, *end;
> > >+ char cur_idx[256];
> > >+ char type_idx[256];
> > >+
> > >+ start = visit_path_str;
> > >+ sprintf(type_idx, "%d", i);
> >
> > Let's avoid sprintf completely.
> >
> > >+ while (start) {
> > >+ end = strchr(start, ':');
> > >+ if (!end) {
> > >+ break;
> > >+ }
> > >+ snprintf(cur_idx, end - start + 1, "%s", start);
> > >+ start = end + 1;
> > >+ /* if the type was already extended in parent node,
> > >+ * we don't extend it again to avoid dead loop. */
> > >+ if (!strcmp(cur_idx, type_idx)) {
> > >+ return NULL;
> > >+ }
> > >+ }
> >
> > Using array of int will get you a simpler code here.
> >
> > Furthermore, you could avoid linear lookup by using a bitmap beside
> > visit_path_str to keep track of what is visited.
> >
> > >+ /* push index to visit_path_str before extending */
> > >+ push_id(i);
> > >+ }
> > >+ if (!strcmp(str, qstring_copy_str(value))) {
> > >+
> > >+ return qobject_from_json(qmp_schema_table[i]);
> > >+ }
> > >+ }
> > >+ return NULL;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_dict(QObject *data);
> > >+static DataObject *visit_qobj_list(QObject *data);
> > >+
> > >+/* extend defined type to json object */
> > >+static DataObject *extend_type(const char *str)
> > >+{
> > >+ QObject *data;
> > >+ DataObject *obj;
> > >+
> > >+ data = get_definition(str, true);
> > >+
> > >+ if (data) {
> > >+ obj = visit_qobj_dict(data);
> > >+ pop_id();
> > >+ } else {
> > >+ obj = g_malloc0(sizeof(struct DataObject));
> > >+ obj->kind = DATA_OBJECT_KIND_REFERENCE_TYPE;
> > >+ obj->reference_type = g_malloc0(sizeof(String));
> > >+ obj->reference_type->str = g_strdup(str);
> > >+ }
> > >+
> > >+ return obj;
> > >+}
> > >+
> > >+static DataObjectMemberList *list_to_memberlist(QObject *data)
> > >+{
> > >+ DataObjectMemberList *mem_list, *entry, *last_entry;
> > >+ QList *qlist;
> > >+ const QListEntry *lent;
> > >+
> > >+ qlist = qobject_to_qlist(data);
> >
> > Bad indentation starting from here.
> >
> > >+
> > >+ mem_list = NULL;
> > >+ for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >+ entry = g_malloc0(sizeof(DataObjectMemberList *));
> >
> > Pointer size allocated instead of structure.
> >
> > >+ entry->value = g_malloc0(sizeof(DataObjectMember));
> > >+ entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> > >+
> > >+ if (get_definition(qstring_copy_str(lent->value), false)) {
> > >+ entry->value->type->kind =
> > >DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+ entry->value->has_recursive = true;
> > >+ entry->value->recursive = true;
> > >+ entry->value->type->extend =
> > >+ extend_type(qstring_copy_str(lent->value));
> > >+ } else {
> > >+ entry->value->type->kind =
> > >+ DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > >+ entry->value->has_recursive = true;
> > >+ entry->value->recursive = false;
> > >+ entry->value->type->reference =
> > >+ g_strdup(qstring_copy_str(lent->value));
> > >+ }
> > >+
> > >+ entry->next = NULL;
> > >+ if (!mem_list) {
> > >+ mem_list = entry;
> > >+ } else {
> > >+ last_entry->next = entry;
> > >+ }
> > >+ last_entry = entry;
> > >+ }
> > >+ return mem_list;
> > >+}
> > >+
> > >+static DataObjectMemberList *dict_to_memberlist(QObject *data)
> > >+{
> > >+ DataObjectMemberList *mem_list, *entry, *last_entry;
> > >+ QDict *qdict;
> > >+ const QDictEntry *dent;
> > >+
> > >+ qdict = qobject_to_qdict(data);
> > >+
> > >+ mem_list = NULL;
> > >+ for (dent = qdict_first(qdict); dent; dent = qdict_next(qdict,
> > >dent)) {
> > >+ entry = g_malloc0(sizeof(DataObjectMemberList *));
> >
> > Same here.
> >
> > >+ entry->value = g_malloc0(sizeof(DataObjectMember));
> > >+
> > >+ entry->value->type = g_malloc0(sizeof(DataObjectMemberType));
> > >+
> > >+ if (dent->value->type->code == QTYPE_QDICT) {
> > >+ entry->value->type->kind =
> > >DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+ entry->value->type->extend = visit_qobj_dict(dent->value);
> > >+ } else if (dent->value->type->code == QTYPE_QLIST) {
> > >+ entry->value->type->kind =
> > >DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+ entry->value->type->extend = visit_qobj_list(dent->value);
> > >+ } else if (get_definition(qstring_copy_str(dent->value),
> > >false)) {
> > >+ entry->value->type->kind =
> > >DATA_OBJECT_MEMBER_TYPE_KIND_EXTEND;
> > >+ entry->value->has_recursive = true;
> > >+ entry->value->recursive = true;
> > >+ entry->value->type->extend =
> > >+ extend_type(qstring_copy_str(dent->value));
> > >+ } else {
> > >+ entry->value->type->kind =
> > >+ DATA_OBJECT_MEMBER_TYPE_KIND_REFERENCE;
> > >+ entry->value->has_recursive = true;
> > >+ entry->value->recursive = false;
> > >+ entry->value->type->reference =
> > >+ g_strdup(qstring_copy_str(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);
> > >+ }
> > >+
> > >+ entry->next = NULL;
> > >+ if (!mem_list) {
> > >+ mem_list = entry;
> > >+ } else {
> > >+ last_entry->next = entry;
> > >+ }
> > >+ last_entry = entry;
> > >+ }
> > >+ return mem_list;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_list(QObject *data)
> > >+{
> > >+ DataObject *obj;
> > >+
> > >+ obj = g_malloc0(sizeof(struct DataObject));
> > >+ obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> > >+ obj->anonymous_struct = g_malloc0(sizeof(struct
> > >+ DataObjectAnonymousStruct));
> > >+ obj->anonymous_struct->data = list_to_memberlist(data);
> > >+
> > >+ return obj;
> > >+}
> > >+
> > >+static strList *get_str_list(QObject *data)
> > >+{
> > >+ strList *str_list, *last_str_entry, *str_entry;
> > >+ QList *qlist;
> > >+ const QListEntry *lent;
> > >+
> > >+ qlist = qobject_to_qlist(data);
> > >+ str_list = NULL;
> > >+ for (lent = qlist_first(qlist); lent; lent = qlist_next(lent)) {
> > >+ str_entry = g_malloc0(sizeof(strList *));
> >
> > And here.
> >
> > >+ str_entry->value = g_strdup(qstring_copy_str(lent->value));
> > >+ str_entry->next = NULL;
> > >+ if (!str_list) {
> > >+ str_list = str_entry;
> > >+ } else {
> > >+ last_str_entry->next = str_entry;
> > >+ }
> > >+ last_str_entry = str_entry;
> > >+ }
> > >+
> > >+ return str_list;
> > >+}
> > >+
> > >+static DataObject *visit_qobj_dict(QObject *data)
> > >+{
> > >+ DataObject *obj;
> > >+ QObject *subdata;
> > >+ QDict *qdict;
> > >+
> > >+ qdict = qobject_to_qdict(data);
> > >+ assert(qdict != NULL);
> > >+ obj = g_malloc0(sizeof(*obj));
> > >+
> > >+ if (qdict_get(qdict, "command")) {
> > >+ obj->kind = DATA_OBJECT_KIND_COMMAND;
> > >+ obj->has_name = true;
> > >+ obj->name = g_strdup(qstring_copy_str(qdict_get(qdict,
> > >"command")));
> > >+ obj->command = g_malloc0(sizeof(struct DataObjectCommand));
> > >+
> > >+ subdata = qdict_get(qdict, "data");
> > >+ if (subdata && subdata->type->code == QTYPE_QDICT) {
> > >+ obj->command->has_data = true;
> > >+ obj->command->data = dict_to_memberlist(subdata);
> > >+ } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> > >+ abort();
> > >+ } else if (subdata) {
> > >+ obj->command->has_data = true;
> > >+ obj->command->data =
> > >+
> > >dict_to_memberlist(get_definition(qstring_copy_str(subdata),
> > >+ true));
> > >+ pop_id();
> > >+ }
> > >+
> > >+ subdata = qdict_get(qdict, "returns");
> > >+ if (subdata && subdata->type->code == QTYPE_QDICT) {
> > >+ abort();
> > >+ } else if (subdata && subdata->type->code == QTYPE_QLIST) {
> > >+ obj->command->has_returns = true;
> > >+ obj->command->returns = visit_qobj_list(subdata);
> > >+ } else if (subdata && subdata->type->code == QTYPE_QSTRING) {
> > >+ obj->command->has_returns = true;
> > >+ obj->command->returns =
> > >extend_type(qstring_copy_str(subdata));
> > >+ }
> > >+
> > >+ subdata = qdict_get(qdict, "gen");
> > >+ if (subdata && subdata->type->code == QTYPE_QSTRING) {
> > >+ obj->command->has_gen = true;
> > >+ if (!strcmp(qstring_copy_str(subdata), "no")) {
> > >+ obj->command->gen = false;
> > >+ } else {
> > >+ obj->command->gen = true;
> > >+ }
> > >+ }
> > >+ } else if (qdict_get(qdict, "union")) {
> > >+ obj->kind = DATA_OBJECT_KIND_UNIONOBJ;
> > >+ obj->has_name = true;
> > >+ obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "union")));
> > >+ obj->unionobj = g_malloc0(sizeof(struct DataObjectUnion));
> > >+ subdata = qdict_get(qdict, "data");
> > >+ obj->unionobj->data = dict_to_memberlist(subdata);
> > >+ } else if (qdict_get(qdict, "type")) {
> > >+ obj->kind = DATA_OBJECT_KIND_TYPE;
> > >+ obj->has_name = true;
> > >+ obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "type")));
> > >+ obj->type = g_malloc0(sizeof(struct DataObjectType));
> > >+ subdata = qdict_get(qdict, "data");
> > >+ obj->type->data = dict_to_memberlist(subdata);
> > >+ } else if (qdict_get(qdict, "enum")) {
> > >+ obj->kind = DATA_OBJECT_KIND_ENUMERATION;
> > >+ obj->has_name = true;
> > >+ obj->name = g_strdup(qstring_copy_str(qdict_get(qdict, "enum")));
> > >+ obj->enumeration = g_malloc0(sizeof(struct
> > >DataObjectEnumeration));
> > >+ subdata = qdict_get(qdict, "data");
> > >+ obj->enumeration->data = get_str_list(subdata);
> > >+ } else {
> > >+ obj->kind = DATA_OBJECT_KIND_ANONYMOUS_STRUCT;
> > >+ obj->anonymous_struct = g_malloc0(sizeof(struct
> > >+
> > >DataObjectAnonymousStruct));
> > >+ obj->anonymous_struct->data = dict_to_memberlist(data);
> > >+ }
> > >+
> > >+ return obj;
> > >+}
> > >+
> > >+DataObjectList *qmp_query_qmp_schema(Error **errp)
> > >+{
> > >+ DataObjectList *list, *last_entry, *entry;
> > >+ QObject *data;
> > >+ int i;
> > >+
> > >+ list = NULL;
> > >+ for (i = 0; qmp_schema_table[i]; i++) {
> > >+ data = qobject_from_json(qmp_schema_table[i]);
> > >+ assert(data != NULL);
> > >+
> > >+ entry = g_malloc0(sizeof(DataObjectList *));
> >
> > And here.
> >
> > >+ memset(visit_path_str, 0, sizeof(visit_path_str));
> > >+ entry->value = visit_qobj_dict(data);
> > >+ entry->next = NULL;
> > >+ if (!list) {
> >
> > Why not use a pointer to pointer to avoid this if branch?
>
> Will fix it.
>
> > >+ list = entry;
> > >+ } else {
> > >+ last_entry->next = entry;
> > >+ }
> > >+ last_entry = entry;
> > >+ }
> > >+
> > >+ return list;
> > >+}
> > >+
> > > void qmp_add_client(const char *protocol, const char *fdname,
> > > bool has_skipauth, bool skipauth, bool has_tls, bool
> > > tls,
> > > Error **errp)
> > >
> >
> > Fam
>