From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41251) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWSgt-0006uW-1f for qemu-devel@nongnu.org; Tue, 23 Sep 2014 12:11:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWSgo-0008QN-Cm for qemu-devel@nongnu.org; Tue, 23 Sep 2014 12:11:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40785) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWSgo-0008Ol-5J for qemu-devel@nongnu.org; Tue, 23 Sep 2014 12:11:42 -0400 Message-ID: <54219BB6.3070007@redhat.com> Date: Tue, 23 Sep 2014 10:11:34 -0600 From: Eric Blake MIME-Version: 1.0 References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-9-git-send-email-eblake@redhat.com> <87tx3ygzbc.fsf@blackfin.pond.sub.org> In-Reply-To: <87tx3ygzbc.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="hcHO6uW4sop54Tm4Xe7bjdMgpVRASfsDj" Subject: Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Fam Zheng , qemu-devel@nongnu.org, wenchaoqemu@gmail.com, Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hcHO6uW4sop54Tm4Xe7bjdMgpVRASfsDj Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/23/2014 08:56 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The previous commit demonstrated that the generator overlooked some >> fairly basic broken expressions: >> - missing metataype >> - metatype key has a non-string value >> - unknown key in relation to the metatype >> - conflicting metatype (this patch treats the second metatype as an >> unknown key of the first key visited, which is not necessarily the >> first key the user typed) >=20 > The whole thing is a Saturday afternoon hack, with extra hacks bolted o= n > left & right. And this series is a sequence of MY Saturday afternoon hacks in cleaning it up :) >=20 >> Add check_keys to cover these situations, and update testcases to >> match. A couple other tests (enum-missing-data, indented-expr) had >> to change since the validation added here occurs so early. >> >> While valid .json files won't trigger any of these cases, we might >> as well be nicer to developers that make a typo while trying to add >> new QAPI code. >> >> Signed-off-by: Eric Blake >=20 >> +def check_keys(expr_elem, meta, required, optional=3D[]): >> + expr =3D expr_elem['expr'] >> + info =3D expr_elem['info'] >> + name =3D expr[meta] >=20 > Caller ensures expr[meta] exists. Okay. >=20 >> + if not isinstance(name, basestring): >=20 > I'm a Python noob: why basestring and not str? Me too. No clue. Copy and paste from existing code. http://git.qemu.org/?p=3Dqemu.git;a=3Dblob;f=3Dscripts/qapi.py;h=3D77d46a= a;hb=3D769188d3b#l361 >=20 >> + raise QAPIExprError(info, >> + "%s key must have a string value" % meta)= >> + expr_elem['name'] =3D name >=20 > Where is this used? Hmm, I know I used it at one point in my series (to be able to print the name of an expression in check_type added in 13/19, without having to repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc. in multiple places). Although in my refactoring, I may have eliminated the need for it after all. If it's not in use at the end of the series, I can drop it. >=20 >> + required.append(meta) >=20 > Ugly side effect. To avoid, either make a new list here >=20 > required =3D required + [ meta ] >=20 > or do nothing here and... >=20 >> + for (key, value) in expr.items(): >> + if not key in required and not key in optional: >=20 > ... add "and key !=3D meta" to this condition. Shows my inexperience in python. Sure, I can fix. Looks like I get to spin a v5. >=20 > This hunk is easier to review with whitespace ignored: Indeed. Alas, python is a stickler about whitespace reindentation. Since I have to respin, I'll probably split into two pieces (one with a no-op change that reindents, the other for the improvements). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --hcHO6uW4sop54Tm4Xe7bjdMgpVRASfsDj 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 iQEcBAEBCAAGBQJUIZu2AAoJEKeha0olJ0NquvUH/3QXnxy9B0bTkD9A5L1Q2ZfM OWh80QyLshQfChEaro4s9IfZqAkDwclD9qngNfTTRKJavlx0jiDpZV3dDX6A/coY A/hm5ka57mwXh7od93S9vbGNIHtEP1GOgGKlgxqaxcxtY+PoYeugfA7FCIKe/hR3 N3EgaxtO5zOL4YbbX9C0piM2IQPEAYflLOgH7nsWZlJaEnvqxIJ5fccPnsJnbG9b /1YWD44bi998v0Bs0FXK2aGw8MbMg4uDv5BzngfTwi1Bzv2PKqJoxUIsO15NvUDu FDD6C3JcsjGhLvKP0v/OhUIoCWGuIntdqAPPJcnsdFgXhfyuiM5olqZFSOAa+fY= =Qaha -----END PGP SIGNATURE----- --hcHO6uW4sop54Tm4Xe7bjdMgpVRASfsDj--