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@xxxxxxxxxx> > > >>>--- > > >>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list