From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55257) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlUvR-0005IN-HQ for qemu-devel@nongnu.org; Tue, 26 Nov 2013 21:32:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlUvN-0007w2-Ad for qemu-devel@nongnu.org; Tue, 26 Nov 2013 21:32:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45231) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlUvM-0007vy-Vx for qemu-devel@nongnu.org; Tue, 26 Nov 2013 21:32:21 -0500 Date: Wed, 27 Nov 2013 10:32:17 +0800 From: Amos Kong Message-ID: <20131127023217.GA6629@amosk.info> References: <1373971062-28909-1-git-send-email-akong@redhat.com> <1373971062-28909-3-git-send-email-akong@redhat.com> <51E9B81C.2090105@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="MGYHOYXEY6WxJCY8" Content-Disposition: inline In-Reply-To: <51E9B81C.2090105@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 2/2] full introspection support for QMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: "kwolf@redhat.com" , aliguori@us.ibm.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com --MGYHOYXEY6WxJCY8 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 19, 2013 at 04:05:16PM -0600, Eric Blake wrote: > On 07/16/2013 04:37 AM, Amos Kong wrote: > > Introduces new monitor command to query QMP schema information, > > the return data is a dynamical and nested dict/list, it contains >=20 > s/dynamical/dynamic/ >=20 > > the useful metadata to help management to check feature support, > > QMP commands detail, etc. > >=20 > > I added a document for QMP introspection support. > > (docs/qmp-full-introspection.txt) >=20 > Yay - docs make a world of difference. >=20 > >=20 > > We need to parse all commands json definition, and generated a >=20 > mixing tense ("need" present, "generated" past); for commit messages, > present tense is generally appropriate. Thus: >=20 > s/generated/generate/ >=20 > > dynamical tree, QMP infrastructure will convert the tree to >=20 > s/dynamical/dynamic/ >=20 > > json string and return to QMP client. > >=20 > > So here I defined a 'DataObject' type in qapi-schema.json, > > it's used to describe the dynamical dictionary/list/string. >=20 > s/dynamical/dynamic/ >=20 > >=20 > > { 'type': 'DataObject', > > 'data': { '*key': 'str', '*type': 'str', '*data': ['DataObject'] } } > >=20 > > Not all the keys in data will be used. > > # List: type > > # Dict: key, type > > # nested List: type, data > > # nested Dict: key, type, data >=20 > I wonder if we can take advantage of Kevin's work on unions to make this > MUCH easier to use: >=20 > https://lists.gnu.org/archive/html/qemu-devel/2013-07/msg01407.html >=20 > { 'enum': 'DataObjectType', > 'data': [ 'command', 'struct', 'union', 'enum' ] } > # extend to add 'event' later >=20 > { 'type': 'DataObjectBase', > 'data': { 'name': 'str' } } >=20 > { 'union': 'DataObjectMemberType', > 'discriminator': {}, > 'data': { 'reference': 'str', > 'inline': 'DataObject' } } >=20 > { 'type': DataObjectMember', > 'data': { 'name': 'str', 'type': 'DataObjectMemberType', > '*optional': 'bool', '*recursive': 'bool' } } >=20 > { 'type': 'DataObjectStruct', > 'data': { 'data': [ 'DataObjectMember' ] } } > { 'type': 'DataObjectEnum', > 'data': { 'data': [ 'str' ] } } > { 'type': 'DataObjectUnion', > 'data': { 'data': [ 'DataObjectMember' ], '*base': 'str', > '*discriminator': 'str' } } > { 'type': 'DataObjectCommand', > 'data': { 'data': [ 'DataObjectMember' ], '*returns': 'DataObject' } } =20 > { 'union': 'DataObject', > 'base': 'DataObjectBase', > 'discriminator': 'type', > 'data': { > 'struct': 'DataObjectStruct', > 'enum': 'DataObjectEnum', > 'union': 'DataObjectUnion', > 'command': 'DataObjectCommand', > 'array': {} } In my patch, I used a _dictionary_ to describe this kind of thing 1) dict, 2) list, 3) str The above line is used for Dictionary or List, it should be: 'array': ['DataObject'] But I touched a new error: qapi-visit.c: In function =E2=80=98visit_type_DataObject=E2=80=99: qapi-visit.c:7255:29: error: implicit declaration of function =E2=80=98visi= t_type_DataObjectList_fields=E2=80=99 [-Werror=3Dimplicit-function-declarat= ion] visit_type_DataObjectList_fields(m, &(*obj)->a= rray, &err); ---- So I try to defined=20 { 'type': 'DataObjectArray', 'data': ['DataObject'] } 'DataObjectArrayType' } } { 'union': 'DataObject', 'base': 'DataObjectBase', 'discriminator': 'name', 'data': { 'array': 'DataObjectArray', got error: Traceback (most recent call last): File "/home/devel/qemu/scripts/qapi-types.py", line 471, in ret +=3D generate_struct(expr) + "\n" File "/home/devel/qemu/scripts/qapi-types.py", line 101, in generate_stru= ct ret +=3D generate_struct_fields(members) File "/home/devel/qemu/scripts/qapi-types.py", line 67, in generate_struc= t_fields for argname, argentry, optional, structured in parse_args(members): File "/home/devel/qemu/scripts/qapi.py", line 197, in parse_args argentry =3D typeinfo[member] TypeError: list indices must be integers, not str ---- a new definition: { 'enum': 'DataObjectArrayType', 'data': ['Dictionary', 'List'] } { 'type': 'DataObjectArray', 'data': {'data': ['DataObject'], 'type': 'DataObjectArrayType' } } { 'union': 'DataObject', 'base': 'DataObjectBase', 'discriminator': 'name', 'data': { 'array': 'DataObjectArray', ----- In my V2, I parse the schema just according the Format attribute (dict, str= , list) Eric suggested defination is wonderful, but it's not flexible as mine ;-) The data type, format (dict/str/list), more matadata should be considered. It makes the parse very complex. I have to simple it, the matadata will also provided, just make the parse work easyer. Libvirt can still get good info as using Eric's defination. Thanks, Amos > > The DataObject is described in docs/qmp-full-introspection.txt in > > detail. > >=20 > > The following content gives an example of query-tpm-types: > >=20 > > ## Define example in qapi-schema.json: > >=20 > > { 'enum': 'TpmType', 'data': [ 'passthrough' ] } > > { 'command': 'query-tpm-types', 'returns': ['TpmType'] } >=20 > It might be more useful to have a (made-up) example that shows as many > details as possible - a command that takes arguments and has a return > type will exercise more of the code paths than a query command with only > a return. >=20 > >=20 > > ## Returned description: > >=20 > > { > > "name": "query-tpm-types", > > "type": "Command", > > "returns": [ > > { > > "type": "TpmType", > > "data": [ > > { > > "type": "passthrough" > > } > > ] >=20 > I need a way to know the difference between a TpmType returned directly > in a dict, vs. a return containing an array of TpmType. >=20 > > } > > ] > > }, >=20 > Thus, using the discriminated union I set out above, I would return > something like: > { "name": "TpmType", "type": "enum", > "data": [ "passthrough" ] }, > { "name": "query-tpm-types", "type": "command", > "data": [ ], > "returns": { "name": "TpmType", "type": "array" } } >=20 > >=20 > > 'TpmType' is a defined type, it will be extended in returned > > description. [ 'passthrough' ] is a list, so 'type' and 'data' > > will be used. > >=20 > > TODO: > > We can add events definations to qapi-schema.json by another >=20 > s/definations/definitions/ >=20 > > patch, then event can also be queried. > >=20 > > Introduce another command 'query-qga-schema' to query QGA schema > > information, it's easy to add this support with current current > > patch. >=20 > Such a command would be part of the QGA interface, not a QMP command. > But yes, it is needed, and the ideal patch series from you will include > that patch as part of the series. But I don't mind waiting until we get > the interface nailed down before you actually implement the QGA repeat > of the interface. >=20 > >=20 > > Signed-off-by: Amos Kong > > --- > > docs/qmp-full-introspection.txt | 143 +++++++++++++++++++ > > qapi-schema.json | 69 +++++++++ > > qmp-commands.hx | 39 +++++ > > qmp.c | 307 ++++++++++++++++++++++++++++++++= ++++++++ > > 4 files changed, 558 insertions(+) > > create mode 100644 docs/qmp-full-introspection.txt > >=20 > > diff --git a/docs/qmp-full-introspection.txt b/docs/qmp-full-introspect= ion.txt > > new file mode 100644 > > index 0000000..cc0fb80 > > --- /dev/null > > +++ b/docs/qmp-full-introspection.txt > > @@ -0,0 +1,143 @@ > > +=3D full introspection support for QMP =3D > > + >=20 > Is it worth merging this into the existing qapi-code-gen.txt, or at > least having the two documents refer to one another, since they deal > with related concepts (turning the .json file into generated code)? >=20 > > +=3D=3D Purpose =3D=3D > > + > > +Add a new interface to provide QMP schema information to management, >=20 > s/a new/an/ - after a year, the interface won't be new, but this doc > will still be relevant. >=20 > > +the return data is a dynamical and nested dict/list, it contains >=20 > s/dynamical/dynamic/ >=20 > > +the useful metadata to help management to check feature support, > > +QMP commands detail, etc. > > + > > +=3D=3D Usage =3D=3D > > + > > +Execute QMP command: > > + > > + { "execute": "query-qmp-schema" } > > + > > +Returns: > > + > > + { "return": [ > > + { > > + "name": "query-name", > > + "type": "Command", > > + "returns": [ > > + { > > + "key": "*name", > > + "type": "str" > > + } > > + ] >=20 > Are we trying to use struct names where they exist for compactness, or > trying to inline-expand everything as far as possible until we run into > self-referential recursion? >=20 > > + }, > > + ... > > + } > > + > > +The whole schema information will be returned in one go, it contains > > +commands and event. It doesn't support to be filtered by type or name. >=20 > s/event/events/ > s/It/At present, it/ > s/to be filtered/filtering/ >=20 > > + > > +We have four types (ImageInfo, BlockStats, PciDeviceInfo, SchemaData) >=20 > This list will get out of date quickly. I'd just word it generically: >=20 > We have several self-referential types >=20 > > +that uses themself in their own define data directly or indirectly, >=20 > s/uses themself/use themselves/ >=20 > > +we will not repeatedly extend them to avoid dead loop. >=20 > Would it be worth a flag in the QMP output when a type is not being > further expanded because it is a nested occurrence of itself? That is, > given my proposed layout above, ImageInfo would turn into something like: >=20 > { "name": "ImageInfo", "type": "struct", > "data": [ { "name": "filename", "type", "str" }, > ... > { "name": "backing-image", "type": "ImageInfo", > "optional": true, "recursive": true } ] }, >=20 > > + > > +=3D=3D more description of 'DataObject' type =3D=3D > > + > > +We use 'DataObject' to describe dynamical data struct, it might be >=20 > s/dynamical/dynamic/ >=20 > > +nested dictionary, list or string. > > + > > +'DataObject' itself is a arbitrary and nested dictionary, the > > +dictionary has three keys ('key', 'type', 'data'), 'key' and >=20 > spacing is odd. >=20 > > +'data' are optional. > > + > > +* For describing Dictionary, we set the key to 'key', and set the > > + value to 'type' > > +* For describing List, we don't set 'key', just set the value to > > + 'type' >=20 > Again, if you use the idea of a discriminated union, you don't need > quite the complexity in describing this: dictionaries are listed with > key/type/optional tuples, lists (enums) are listed with just an array of > strings, and the QAPI perfectly described that difference by the > discriminator telling you 'struct' vs. 'enum'. >=20 > > +* If the value of dictionary or list is non-native type, we extend > > + the non-native type to dictionary, set it to 'data', and set the > > + non-native type's name to 'type'. >=20 > Again, I tried to set up the QAPI that describes something that uses an > anonymous union that either gives a string (the name of a primitive or > already-defined type) or a struct (a recursion into another layer of > struct describing the structure of that type in line). >=20 > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 7b9fef1..cf03391 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -3679,3 +3679,72 @@ > > '*cpuid-input-ecx': 'int', > > 'cpuid-register': 'X86CPURegister32', > > 'features': 'int' } } > > + > > +## > > +# @DataObject > > +# > > +# Details of a data object, it can be nested dictionary/list > > +# > > +# @key: #optional Data object key > > +# > > +# @type: Data object type name > > +# > > +# @optional: #optional whether the data object is optional >=20 > mention that the default is false. >=20 > > +# > > +# @data: #optional DataObject list, can be a dictionary or list type >=20 > so if 'data' is present, we are describing @type in more detail; if it > is absent, then @type is primitive. >=20 > > +# > > +# Since: 1.6 > > +## > > +{ 'type': 'DataObject', > > + 'data': { '*key': 'str', '*type': 'str', '*optional': 'bool', '*data= ': ['DataObject'] } } > > + >=20 > '*type' should be an enum type, not open-coded string. >=20 >=20 > > +## > > +# @SchemaMetaType > > +# > > +# Possible meta types of a schema entry > > +# > > +# @Command: QMP command > > +# > > +# @Type: defined new data type > > +# > > +# @Enumeration: enumeration data type > > +# > > +# @Union: union data type > > +# > > +# Since: 1.6 > > +## > > +{ 'enum': 'SchemaMetaType', > > + 'data': ['Command', 'Type', 'Enumeration', 'Union'] } >=20 > Do we need to start these with a capital? On the other hand, having > them as capitals may make it easier to ensure we are outputting correct > information. > > + > > +## > > +# @SchemaEntry > > +# > > +# Details of schema items > > +# > > +# @type: Entry's type in string format > > +# > > +# @name: Entry name > > +# > > +# @data: #optional list of DataObject. This can have different meaning > > +# depending on the 'type' value. For example, for a QMP command, > > +# this member contains an argument listing. For an enumeration, > > +# it contains the enum's values and so on >=20 > This argues for making it a union type. >=20 > > +# > > +# @returns: #optional list of DataObject, return data after executing > > +# QMP command > > +# > > +# Since: 1.6 > > +## > > +{ 'type': 'SchemaEntry', 'data': { 'type': 'SchemaMetaType', > > + 'name': 'str', '*data': ['DataObject'], '*returns': ['DataObject'] }= } > > + > > +## > > +# @query-qmp-schema > > +# > > +# Query QMP schema information > > +# > > +# Returns: list of @SchemaEntry. Returns an error if json string is in= valid. >=20 > If you don't take any arguments, then the "returns an error" statement > is impossible. >=20 > > +# > > +# Since: 1.6 > > +## > > +{ 'command': 'query-qmp-schema', 'returns': ['SchemaEntry'] } > > diff --git a/qmp-commands.hx b/qmp-commands.hx > > index e075df4..e3cbe93 100644 > > --- a/qmp-commands.hx > > =20 > > +/* > > + * 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]; >=20 > Is a fixed width buffer really the best solution, or does glib offer us > something better? For example, a hash table, or even a growable string. >=20 > > + > > + for (i =3D 0; qmp_schema_table[i]; i++) { > > + data =3D qobject_from_json(qmp_schema_table[i]); > > + assert(data !=3D NULL); > > + > > + qdict =3D qobject_to_qdict(data); > > + assert(qdict !=3D NULL); > > + > > + ent =3D qdict_first(qdict); > > + if (!qdict_get(qdict, "enum") && !qdict_get(qdict, "type") > > + && !qdict_get(qdict, "union")) { > > + continue; > > + } >=20 > Why are we doing the work in C code, every time the command is called? > Wouldn't it be more efficient to do the work in python code, at the time > the qmp_schema_table C code is first generated, so that the generated > code is already in the right format? >=20 > > +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 =3D NULL; > > + for (i =3D 0; qmp_schema_table[i]; i++) { > > + data =3D qobject_from_json(qmp_schema_table[i]); > > + assert(data !=3D NULL); > > + > > + qdict =3D qobject_to_qdict(data); > > + assert(qdict !=3D NULL); > > + > > + if (qdict_get(qdict, "command")) { > > + info =3D g_malloc0(sizeof(*info)); > > + info->type =3D SCHEMA_META_TYPE_COMMAND; > > + info->name =3D strdup(qdict_get_str(qdict, "command")); > > + } else { > > + continue; > > + } > > + >=20 > Don't we want to also output types (struct, union, enum) and eventually > events, not just commands? >=20 > I hope we're converging, but I'm starting to worry that we won't have a > design in place in time for the 1.6 release. >=20 > --=20 > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >=20 --MGYHOYXEY6WxJCY8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iQIcBAEBAgAGBQJSlVmxAAoJELxSv6I5vP9jMRgP/0LH5ZrmTLTNqRNmbeyets4/ 6B4rc3zMdhZu1yGsCEPx2E93EF5Xe4HnRIXBPwyz2bRR0/fm+05rhkNMVH7nngAO wGZFLBmoVtdkPtYqgKeMdkOv5o96x/pYlmVWtO/8W95V4wTPxmro7qdXTp4qdRxj 42j8Wq3O73xzrdfjvy+7OB7dLdtPU2N7DLQ+NL9fXUA+qtPu6RVmnLgtba5muYH1 tvFQ++O4CXQk4FTW7aG+WeILyynoTUQOK8xSWIayj7BMClVVxnqAlIbw+vHj4vgK ioyrNbmEnyyBHoJm+uNB+qnmyMtReJ4GW09H9VTRijKG6UQPlETvpyv1MZekmvLn Nck48d+x793Fl915iIc7gEH56DZYHE0Y5D5TXTYDzWth+gcTDCZbK1sZYNJCTKFF /yzHlIVe788k+VdH8cXORF6zu6C24LoYa7fcFydgaHf4EZ7eqDDpIXD+IXGdAFgh b13HHU6FZhnXeoRMgVOFqEOJs+B+2gMmYtAQ7oXgsAvTodjWcOfX2PK1Qo4smVV5 imdOwZmXx6AFqI/gr9K+555TPRTuAo7WQVivjl3PmiwUgwOvdnqr5PLs5oVDTPPB ujJJMhRqtWSdl8N5A9d6+Zoq+lfYXep9sNRbutiaI7XdAX7lOtW6dlwUc+QEANym aU0kmnhSSj4MsxDwuclU =GBnv -----END PGP SIGNATURE----- --MGYHOYXEY6WxJCY8--