From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUiO8-0006UH-3z for qemu-devel@nongnu.org; Mon, 31 Mar 2014 16:01:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUiO3-0000gc-E2 for qemu-devel@nongnu.org; Mon, 31 Mar 2014 16:00:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1183) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUiO2-0000gN-J4 for qemu-devel@nongnu.org; Mon, 31 Mar 2014 16:00:51 -0400 Message-ID: <5339C950.8060308@redhat.com> Date: Mon, 31 Mar 2014 14:00:16 -0600 From: Eric Blake MIME-Version: 1.0 References: <20140331191631.21034.25433.stgit@fimbulvetr.bsc.es> <20140331191648.21034.9744.stgit@fimbulvetr.bsc.es> In-Reply-To: <20140331191648.21034.9744.stgit@fimbulvetr.bsc.es> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WkCjkoDnbGeW5o78F4BnJxIvI5tK5DFLU" Subject: Re: [Qemu-devel] [PATCH v6 3/4] qapi: Add a primitive to include other files from a QAPI schema file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?TGx1w61zIFZpbGFub3Zh?= , qemu-devel@nongnu.org Cc: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , Markus Armbruster , Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WkCjkoDnbGeW5o78F4BnJxIvI5tK5DFLU Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/31/2014 01:16 PM, Llu=C3=ADs Vilanova wrote: > Signed-off-by: Llu=C3=ADs Vilanova > --- > docs/qapi-code-gen.txt | 11 +++++++++++ > scripts/qapi.py | 39 ++++++++++++++++++++++++++++++++++----- > 2 files changed, 45 insertions(+), 5 deletions(-) I would consider squashing 3 and 4 together, but not the end of the world to keep them separate. > @@ -75,9 +80,33 @@ class QAPISchema: > =20 > while self.tok !=3D None: > expr_info =3D {'fp': fp, 'line': self.line} > - expr_elem =3D {'expr': self.get_expr(False), > - 'info': expr_info} > - self.exprs.append(expr_elem) > + expr =3D self.get_expr(False) > + if isinstance(expr, dict) and "include" in expr: > + if len(expr) !=3D 1: > + raise QAPIExprError(expr_info, "Invalid 'include' = directive") Not covered in patch 4. Ideally, ALL new error messages also come with a test that exposes the message. > + include_path =3D expr["include"] > + if not isinstance(include_path, str): > + raise QAPIExprError(expr_info, > + 'Expected a file path (string)= , got: %s' > + % include_path) s/path/name/ ("path" implies a series of directories to some people) Good, this one is covered by include-non-file.err > + if not os.path.isabs(include_path): > + include_path =3D os.path.join(self.input_dir, incl= ude_path) > + if not os.path.isfile(include_path): > + raise QAPIExprError( > + expr_info, > + 'Non-existing included file "%s"' % > + include_path) Is this error necessary, or would you get a sane error message by just trying to open the file? s/included/include/ Good, covered by include-no-file.err > + if include_path in self.included: > + raise QAPIExprError(expr_info, "Infinite inclusion= loop: %s" > + % " -> ".join(self.included + > + [include_path]))= Good, include-self-cycle.err covers a one-file loop, and include-cycle.err covers a 3-file loop. Not so good: your error message is an extremely long line: +tests/qapi-schema/include-cycle-c.json:1: Infinite inclusion loop: tests/qapi-schema/include-cycle.json -> tests/qapi-schema/include-cycle-b.json -> tests/qapi-schema/include-cycle-c.json -> tests/qapi-schema/include-cycle.json The formatting in Beno=C3=AEt's series was a little nicer aesthetically: +Inclusion loop detected with file: multi_file_loop_include.json +Path to the broken include is: + multi_file_loop_include.json + multi_loop.json Furthermore, it had the benefit of using the spelling provided by the user, rather than the absolute path to the files. You want to track canonical paths for detecting the loop, but do NOT want to report absolute paths back to the user - instead, it's nicer to report back the names as they spelled it. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WkCjkoDnbGeW5o78F4BnJxIvI5tK5DFLU 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/ iQEcBAEBCAAGBQJTOclQAAoJEKeha0olJ0NqbpgH/2b178QqOKEv4gxgmcOXf6Dv LUGV6HLGKgq7wUe9Kutssv7j0AsPHAYXd1N+NNeN160RNoQN1gQaQFoyylLa7AA0 wccvEkCGV2wETLwZdreH4MqEvBCHixMqusvBfGD2a18p3baVxKoK/yhqL7qDtjtv zEFPa5T19icQHAJsi23D3Q6t6wwjZEuf1LvPdcJ3sxUHJdsvX/T2yDvWH3l+xuE7 cMluyc/NnyADol8OgsvgGnfcQCrE5pKfzpFr1SiGwtkj2IFH2106VNu6O3raDe+L SsjMwSSpzMLTt064fOyXgu58B9fgRzWVMmNljnt2e2MhPz7a2vQ5+fI8Zurj/tA= =W/T/ -----END PGP SIGNATURE----- --WkCjkoDnbGeW5o78F4BnJxIvI5tK5DFLU--