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(a)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