From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42993) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WATfg-0006zr-RY for qemu-devel@nongnu.org; Mon, 03 Feb 2014 19:15:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WATfb-0008N9-T3 for qemu-devel@nongnu.org; Mon, 03 Feb 2014 19:15:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49582) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WATfb-0008N5-Kg for qemu-devel@nongnu.org; Mon, 03 Feb 2014 19:15:19 -0500 Message-ID: <52F03110.8040501@redhat.com> Date: Mon, 03 Feb 2014 17:15:12 -0700 From: Eric Blake MIME-Version: 1.0 References: <1390488396-16538-1-git-send-email-akong@redhat.com> <1390488396-16538-3-git-send-email-akong@redhat.com> In-Reply-To: <1390488396-16538-3-git-send-email-akong@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NGjNnEXP5GqWB0cx6eulgDfWAG74Xnta2" Subject: Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong , qemu-devel@nongnu.org Cc: qiaonuohan@cn.fujitsu.com, lcapitulino@redhat.com, mdroth@linux.vnet.ibm.com, xiawenc@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --NGjNnEXP5GqWB0cx6eulgDfWAG74Xnta2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 01/23/2014 07:46 AM, Amos Kong wrote: > This is a code generator for qapi introspection. It will parse > qapi-schema.json, extend schema definitions and generate a schema > table with metadata, it references to the new structs which we used > to describe dynamic data structs. The metadata will help C code to > allocate right structs and provide useful information to management > to checking suported feature and QMP commandline detail. The schema s/suported/supported/ > table will be saved to qapi-introspect.h. >=20 > The $(prefix) is used to as a namespace to keep the generated code > from one schema/code-generation separated from others so code and > be generated from multiple schemas with clobbering previously > created code. >=20 > Signed-off-by: Amos Kong > --- > .gitignore | 1 + > Makefile | 5 +- > docs/qmp-full-introspection.txt | 17 ++++ > scripts/qapi-introspect.py | 172 ++++++++++++++++++++++++++++++++= ++++++++ I am NOT a python expert. But what I _can_ do is apply this patch and review the generated code for sanity. > 4 files changed, 194 insertions(+), 1 deletion(-) > create mode 100644 scripts/qapi-introspect.py >=20 > +++ b/docs/qmp-full-introspection.txt > @@ -42,3 +42,20 @@ types. > =20 > 'anonymous-struct' will be used to describe arbitrary structs > (dictionary, list or string). > + > +=3D=3D Avoid dead loop in recursive extending =3D=3D I'm still not convinced whether recursive extending is necessary. Let's step back and look at the big picture: if libvirt asks for the ENTIRE schema in one go, then it would be nice to have the representation as compact as possible (minimal time sending data across the wire, minimal memory consumed). Libvirt can then create its own hash table of type references encountered as it consumes the JSON, and do its own recursive lookups by reading from its hash table on an as-needed basis. Recursive expansion avoids the need to look up type references, but explodes the size of the data sent over the wire. On the other hand, if we support filtering by one type, then I agree that getting all details about that type in one command is nicer than having to issue a command, notice unknown type references, then issue followup commands to learn about them. So in this regards, having qemu do the expansion minimizes back-and-forth traffic. BUT, should the expansion be inlined (ie. the return is an array of 1 type, but that type contains several further layers of JSON giving the full expansion of every type referenced), or is it smarter to do the expansion via returning multiple types even though libvirt only asked about one (as an example, return an array of 10 types, where the first array entry is the requested type, and the remaining 9 types fill out the type references mentioned by the first array entry). > + > +We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData) > +that uses themself in their own define data directly or indirectly, s/uses/themself/use themselves/ > +we will not repeatedly extend them to avoid dead loop. s/dead loop/infinite loop/ > + > +We use a 'parents List' to record the visit path, type name of each > +extended node will be saved to the List. > + > +Append type name to the list before extending, and remove type name > +from the list after extending. > + > +If the type name is already extended in parents List, we won't extend > +it repeatedly for avoiding dead loop. > + > +'recursive' indicates if the type is extended or not. Ah, now we get to the definition of an extended type that I was asking about in 1/5. > diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py > new file mode 100644 > index 0000000..03179fa > --- /dev/null > +++ b/scripts/qapi-introspect.py > @@ -0,0 +1,172 @@ > +# > +# QAPI introspection info generator > +# > +# Copyright (C) 2014 Red Hat, Inc. > +# > +# Authors: > +# Amos Kong > +# > +# This work is licensed under the terms of the GNU GPLv2. Any reason you can't use GPLv2+ (that is, use the "or later" clause)? (Red Hat already has blanket approval for any contributions from employees to be relicensed to GPLv2+, but if you copied code from other files with a tighter license and non-RH contributor, it's harder to use that blanket approval, so it's easier to ask you now up front.) > +fdecl.write(mcgen(''' > +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */ > + > +/* > + * Head file to store parsed information of QAPI schema > + * > + * Copyright (C) 2014 Red Hat, Inc. > + * > + * Authors: > + * Amos Kong > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 = or later. Or even license your .py under LGPLv2+, since the generated code is under that license? > +def extend_schema(expr, parents=3D[], member=3DTrue): > + ret =3D {} > + recu =3D 'False' > + name =3D "" > + > + if type(expr) is OrderedDict: > + if not member: > + e =3D expr.popitem(last=3DFalse) > + typ =3D e[0] > + name =3D e[1] > + else: > + typ =3D "anonymous-struct" > + > + if typ =3D=3D 'enum': > + for key in expr.keys(): > + ret[key] =3D expr[key] > + else: > + ret =3D {} > + for key in expr.keys(): > + ret[key], parents =3D extend_schema(expr[key], parents= ) This is where I question whether extend by default is the right action. > + > + elif type(expr) is list: > + typ =3D 'anonymous-struct' > + ret =3D [] > + for i in expr: > + tmp, parents =3D extend_schema(i, parents) > + ret.append(tmp) > + elif type(expr) is str: > + name =3D expr > + if schema_dict.has_key(expr) and expr not in parents: > + parents.append(expr) > + typ =3D schema_dict[expr][1] > + recu =3D 'True' > + ret, parents =3D extend_schema(schema_dict[expr][0].copy()= , > + parents, False) > + parents.remove(expr) > + ret['_obj_recursive'] =3D 'True' > + return ret, parents > + else: > + return expr, parents > + > + return {'_obj_member': "%s" % member, '_obj_type': typ, > + '_obj_name': name, '_obj_recursive': recu, > + '_obj_data': ret}, parents > + What's with the leading underscore? > + > +exprs =3D parse_schema(sys.stdin) > +schema_dict =3D {} > + > +for expr in exprs: > + if expr.has_key('type') or expr.has_key('enum') or expr.has_key('u= nion'): > + e =3D expr.copy() > + We'll eventually need to cover event types. > + first =3D e.popitem(last=3DFalse) > + schema_dict[first[1]] =3D [expr.copy(), first[0]] > + > +fdecl.write('''const char *const qmp_schema_table[] =3D { > +''') > + > +def convert(odict): > + d =3D {} > + for k, v in odict.items(): > + if type(v) is OrderedDict: > + d[k] =3D convert(v) > + elif type(v) is list: > + l =3D [] > + for j in v: > + if type(j) is OrderedDict: > + l.append(convert(j)) > + else: > + l.append(j) > + d[k] =3D l > + else: > + d[k] =3D v > + return d > + > +count =3D 0 > +for expr in exprs: > + fdecl.write(''' /* %s */ > +''' % expr) > + > + expr, parents =3D extend_schema(expr, [], False) > + fdecl.write(''' "%s", > + > +''' % convert(expr)) > + > +fdecl.write(''' NULL }; > + > +#endif > +''') > + > +fdecl.flush() > +fdecl.close() >=20 Like I said, I think my review will be more helpful by looking at the generated file; I'll follow up in another email (probably tomorrow, since it's now late for me) with more comments once I actually finish tha= t. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --NGjNnEXP5GqWB0cx6eulgDfWAG74Xnta2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJS8DERAAoJEKeha0olJ0NqvBIIAIL2z9jS9tojxJU8BkdA65Do v5vcRZzwiHWY5QIJegBcMkO/VGBGVcnrXO2tl5EPe9LboU08gslf8LzpfhWw+QXW bTwMb20iMXfTLZJWyBRcpFLHW1dU6uLFHuHX+n34p0qppvWzBxZqOZOOnqGtJSQ+ zBK6vX0b9XYHNifyCR3Ost1vbhXUV9pzxRXnoLdYEYkKnayDeHrOeUaDFzF1ZVoV rid/q1Rn5cJze/gT3MMBiLmsYBrLfLweWTVX40alPRZ/lqihwOWoftEbiyBOtqWs XrP26qz4gCqIKeMGQ7TM6N8A11K6sSAh2jsvvLYKotGV8vUN26IBWw6OHrtPp8U= =rr0R -----END PGP SIGNATURE----- --NGjNnEXP5GqWB0cx6eulgDfWAG74Xnta2--