From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50925) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYc3C-0007nj-LI for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:35:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYc37-0003Z9-Db for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:35:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYc37-0003Yr-6c for qemu-devel@nongnu.org; Mon, 29 Sep 2014 10:35:37 -0400 Message-ID: <54296E31.2010807@redhat.com> Date: Mon, 29 Sep 2014 08:35:29 -0600 From: Eric Blake MIME-Version: 1.0 References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-14-git-send-email-eblake@redhat.com> <87r3yyohpq.fsf@blackfin.pond.sub.org> In-Reply-To: <87r3yyohpq.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="VGMKSQpwdCg5XOvrvp7PEPO6iR2NSSsnb" Subject: Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types 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) --VGMKSQpwdCg5XOvrvp7PEPO6iR2NSSsnb Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/26/2014 03:26 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Now that we know every expression is valid with regards to >> its keys, we can add further tests that those keys refer to >> valid types. With this patch, all references to a type (the >> 'data': of command, type, union, and event, and the 'returns': >> of command) must resolve to a builtin or another type declared >> by the current qapi parse; this includes recursing into each >> member of a data dictionary. Dealing with '**' and nested >> sub-structs will be done in later patches. >> >> + for (key, value) in data.items(): >> + check_type(expr_info, "member '%s' of %s" % (key, source), va= lue, >> + allow_array=3DTrue, >> + allowed_names=3D['built-in', 'union', 'struct', 'e= num'], >> + allow_dict=3DTrue) >=20 > Hmm, allowed_names isn't about allowed names, it's about allowed > meta-types. Rename? Will do. >> def check_exprs(schema): >> for expr_elem in schema.exprs: >> expr =3D expr_elem['expr'] >> info =3D expr_elem['info'] >> >> - if expr.has_key('union'): >> - check_union(expr, info) >> - if expr.has_key('event'): >> - check_event(expr, info) >> if expr.has_key('enum'): >> check_enum(expr, info) >> + if expr.has_key('union'): >> + check_union(expr, info) >> + if expr.has_key('type'): >> + check_struct(expr, info) >> if expr.has_key('command'): >> check_command(expr, info) >> + if expr.has_key('event'): >> + check_event(expr, info) >=20 > Not related to this patch: this chain of ifs bothers me. The condition= s > should be exclusive, because each expression must have a well-defined > meta-type. If I remember correctly, you actually enforce this elsewher= e > in your series. Do we want to make it obvious in the code here? elif > instead of if, perhaps? Yes, earlier in the series guarantees that by this point, the conditions are now exclusive. It also bothers me that different points in the file are iterating over the 5 key types in different order, I'll clean that up in v5. >> +++ b/tests/qapi-schema/data-array-unknown.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/data-array-unknown.json:2: 'data' for command 'oops= ' references unknown type 'NoSuchType' >=20 > The wording "references type" somehow unduly excites my "reference type= " > synapses :) >=20 > Perhaps something like "Type 'NoSuchType' is unknown" would suffice. > Entirely up to you; feel free to keep the wording as is. I'll play with it. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --VGMKSQpwdCg5XOvrvp7PEPO6iR2NSSsnb 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 iQEcBAEBCAAGBQJUKW4xAAoJEKeha0olJ0NqA1oIAK1u9JXP4wWsakslrvAltkCq Da2BtWWAuhqboqGiC0Gi7JhLz3SegyX4n07yARzXTFkr9kaQs6gh8Ej0fLif6uT9 1Oh9v+nhjA3r0SzBmdDz1A3g7dhYr8Tgy6U7veh1UyEmbJb7tYKH3TGDKLn3jqsB YMfBYt9aqMhqJiuImhTK+hGVtSY9YUV3nON2ZF1cibd0KNfy/bC4lMwevm/r9+Rp pyo93JMp9OfDOX9AOmCWCZgvhK71gOTRiKQGsKNWkPo2xJSoTsTnsWyNMMyQWgnN a6NUr+V6PudyzNwuY9PIfzD9daOuId2+tOLLyLeXHTfaM7LqkXR7yqAZobiKoo8= =pLXq -----END PGP SIGNATURE----- --VGMKSQpwdCg5XOvrvp7PEPO6iR2NSSsnb--