From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTEH8-0002l0-S6 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 13:39:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTEH2-00018r-WF for qemu-devel@nongnu.org; Thu, 27 Mar 2014 13:39:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTEH2-00018l-N9 for qemu-devel@nongnu.org; Thu, 27 Mar 2014 13:39:28 -0400 Message-ID: <53346249.7070705@redhat.com> Date: Thu, 27 Mar 2014 11:39:21 -0600 From: Eric Blake MIME-Version: 1.0 References: <1395934399-18769-1-git-send-email-benoit.canet@irqsave.net> <1395934399-18769-4-git-send-email-benoit.canet@irqsave.net> In-Reply-To: <1395934399-18769-4-git-send-email-benoit.canet@irqsave.net> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="N4HnfwCmIahlolqknUW7edUVrsHebLoLJ" Subject: Re: [Qemu-devel] [PATCH V2 3/3] qapi: Create an include directive for use in the JSON description files. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org Cc: anthony@codemonkey.ws, Benoit Canet , armbru@redhat.com, wenchaoqemu@gmail.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --N4HnfwCmIahlolqknUW7edUVrsHebLoLJ Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/27/2014 09:33 AM, Beno=C3=AEt Canet wrote: > The new directive in the form { 'include': 'path/to/file.json' } will t= rigger the > parsing of path/to/file.json. > The directive will be replaced by the result of the parsing. >=20 > This will allow for easy modularisation of qapi JSON descriptions files= =2E >=20 > Signed-off-by: Benoit Canet > --- > docs/qapi-code-gen.txt | 14 ++++++++++++++ > scripts/qapi.py | 17 ++++++++++++++++- > tests/Makefile | 2 +- > tests/qapi-schema/include.exit | 1 + > tests/qapi-schema/include.json | 4 ++++ > tests/qapi-schema/include.out | 8 ++++++++ > tests/qapi-schema/include/include.json | 7 +++++++ > tests/qapi-schema/include_loop.exit | 1 + > tests/qapi-schema/include_loop.json | 1 + > tests/qapi-schema/include_loop.out | 1 + > 10 files changed, 54 insertions(+), 2 deletions(-) > create mode 100644 tests/qapi-schema/include.err > create mode 100644 tests/qapi-schema/include.exit > create mode 100644 tests/qapi-schema/include.json > create mode 100644 tests/qapi-schema/include.out > create mode 100644 tests/qapi-schema/include/include.json > create mode 100644 tests/qapi-schema/include_loop.err > create mode 100644 tests/qapi-schema/include_loop.exit > create mode 100644 tests/qapi-schema/include_loop.json > create mode 100644 tests/qapi-schema/include_loop.out > =20 > for expr_elem in schema.exprs: > expr =3D expr_elem['expr'] > - if expr.has_key('enum'): > + if expr.has_key('include'): > + prefix =3D os.path.split(path)[0] > + sub_path =3D os.path.join(prefix, expr['include']) Should you validate that expr['include'] is a string, so we know the user didn't do something stupid like { 'include': true } or { 'include': { 'hello':'world' } }. And if you add validation, you also need to add tests. > + elif expr.has_key('enum'): > add_enum(expr['enum'], expr['data']) For that matter (pre-existing, but you suffer from the same problem) - there's no validation against unexpected keys, which means { 'enum':'foo', 'garbage':'blah' } and { 'include':'path', 'garbage':'blah' } both manage to silently ignore the 'garbage' key. If you fix it for 'enum', do that first as a separate commit. > +++ b/tests/qapi-schema/include.json > @@ -0,0 +1,4 @@ > +{ 'enum': 'Status', > + 'data': [ 'good', 'bad', 'ugly' ] } > +{ 'include': './include/include.json' } This tests successful inclusion relative to directory names. > +++ b/tests/qapi-schema/include_loop.json > @@ -0,0 +1 @@ > +{ 'include': 'include_loop.json' } And this tests that self-inclusion is rejected. But still missing tests for: error message when the error occurs in an included file (ideally, the error message should be the location within the included file, not the outer file) error occurring after a successful include (ideally, the lines injected by the included file do NOT upset the line number tracking of the original file) multi-file loops being rejected (a includes b includes a) each include name is relative to the current file even across nested inclusion that changes directory (such as 'a' includes 'include/b' includes '../c', should work when 'a' and 'c' are in the same directory, and NOT try to pick up 'c' in the parent directory of 'a'). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --N4HnfwCmIahlolqknUW7edUVrsHebLoLJ 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/ iQEcBAEBCAAGBQJTNGJJAAoJEKeha0olJ0NqNRIH/jX3NTUhvndR0jTfMQivKYqA 0aNjPzb4IhCLJjTZDhUuzckZvFIGyccCPRaLTVdDhyLn3tt+N0GgAUS6AnnpbBDo RYvgRf0JJfA2azTgc2z+rLlj2JWO7yk7jgofIL/D+OmkT+RwVSLKGOq+A5XHuo2j FkxiwC2YN/jm/TLdsY+2FQvWaI3Qj98XkHHAJXJYI5kkvffGTR61brHOW3hgXpaH 1QKFX3Em5cbt9CcJTLPp/rqyzQWFtljcYOxzICOQUf+OPeoAOcjwgtD0OP+pHaMk HmHP5bao1r/PUkJDAmLTLc2NxmWduikO2MUq+kLs8avilPVir+xN90hHyX0lnwQ= =Dmpi -----END PGP SIGNATURE----- --N4HnfwCmIahlolqknUW7edUVrsHebLoLJ--