From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXAQk-0003pw-Bb for qemu-devel@nongnu.org; Thu, 25 Sep 2014 10:54:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XXAQf-000302-JS for qemu-devel@nongnu.org; Thu, 25 Sep 2014 10:54:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11879) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXAQf-0002zc-BG for qemu-devel@nongnu.org; Thu, 25 Sep 2014 10:53:57 -0400 Message-ID: <54241EB3.5080309@redhat.com> Date: Thu, 25 Sep 2014 07:54:59 -0600 From: Eric Blake MIME-Version: 1.0 References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-13-git-send-email-eblake@redhat.com> <87h9zw9mqy.fsf@blackfin.pond.sub.org> In-Reply-To: <87h9zw9mqy.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="C9RttlS26RpbAUjAn9nj62HI1vdpRF6iG" Subject: Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests 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) --C9RttlS26RpbAUjAn9nj62HI1vdpRF6iG Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/25/2014 01:34 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Demonstrate that the qapi generator silently parses confusing >> types, which may cause other errors later on. Later patches >> will update the expected results as the generator is made stricter. >> >> Signed-off-by: Eric Blake >> --- >> tests/Makefile | 8 ++++++-- >> tests/qapi-schema/data-array-empty.err | 0 >> tests/qapi-schema/data-array-empty.exit | 1 + >> tests/qapi-schema/data-array-empty.json | 2 ++ > [Twelve new tests...] >> create mode 100644 tests/qapi-schema/returns-unknown.err >> create mode 100644 tests/qapi-schema/returns-unknown.exit >> create mode 100644 tests/qapi-schema/returns-unknown.json >> create mode 100644 tests/qapi-schema/returns-unknown.out >=20 > Holy moly! Yeah, this series cleans up a lot of cruft, which means a lot of testing.= >> +++ b/tests/qapi-schema/data-array-empty.json >> @@ -0,0 +1,2 @@ >> +# FIXME: we should reject an array for data if it does not contain a = known type >> +{ 'command': 'oops', 'data': [ ] } >=20 > Do we want to permit anything but a complex type for 'data'? Oh, good question. Probably not (I just tested, and nothing already does that). I'll tighten it in v5 (mostly doc changes, plus a one-liner in 13/19 to remove allow_array=3DTrue when calling check_type for a command data member). >=20 > For what it's worth, docs/qmp/qmp-spec.txt specifies: Ooh, I probably ought to skim that file when making my doc improvements on the front end of the series. >=20 > 'data' of list type / json-array "arguments" is an ordered list of > unnamed parameters. Makes sense, but it isn't how QMP works. Or C for= > that matter. Do we really want to support this in QAPI? No existing command takes a non-dict for "arguments", and the generator probably chokes on it. >=20 > If yes, then 'data': [] means the same thing as 'data': {} or no 'data'= : > no arguments. >=20 > Aside: discussion of list types in qapi-code-gen.txt is spread over the= > places that use them. I feel giving them their own section on the same= > level as complex types etc. would make sense. Good idea, will do in v5. >=20 > 'data' of built-in or enumeration type / json-number or json-string > "arguments" could be regarded as a single unnamed parameter. Even if w= e > want unnamed parameters, the question remains whether we want two > syntactic forms for a single unnamed parameter, boxed in a [ ] and > unboxed. I doubt it. No. We don't (patch 13/19 already forbids them, with no violators found). It's not extensible (well, maybe it is by having some way to mark a dict so that at most one of its keys is the default key to be implied when used in a non-dict form, and all other keys being optional, but that's ugly). >=20 > One kind of types left to discuss: unions. I figure the semantics of a= > simple or flat union type are obvious enough, so we can discuss whether= > we want them. Anonymous unions are a different matter, because they > boil down to a single parameter that need not be json-object. Oooh, I didn't even consider anon unions. We absolutely need to support { 'command': 'foo', 'data': 'FlatUnion' } (blockdev-add, anyone), but you are probably right that we don't want to support { 'command': 'foo', 'data': 'AnonUnion'}, because it would allow "arguments" to be a non-dictionary (unless the AnonUnion has only a dict branch, but then why make it a union?). I'll have to make qapi.py be smarter about regular vs. anon unions - it might be easier by using an actual different keyword for anon unions, because they are so different in nature. (Generated code will be the same, just a tweak to the qapi representation and to qapi.py). I'll play with that for v5. >> +++ b/tests/qapi-schema/data-member-array-bad.json >> @@ -0,0 +1,2 @@ >> +# FIXME: we should reject data if it does not contain a valid array t= ype >> +{ 'command': 'oops', 'data': { 'member': [ { 'nested': 'str' } ] } } >=20 > I'm probably just suffering from temporary denseness here... why is thi= s > example problematic? To me, it looks like a single argument 'member' o= f > type "array of complex type with a single member 'nested' of > builtin-type 'str'". The generator does not have a way to produce a List of an unnamed type. All lists are of named types (or rather, every creation of a named type generates code for both that type and its list counterpart). Maybe we eventually want to support an array of an anonymous type, but the generator doesn't handle it now. So it was easier to forbid it when writing 13/19. >> +# FIXME: we should reject an array return that is not a single type >> +{ 'command': 'oops', 'returns': [ 'str', 'str' ] } >=20 > Do we want to permit anything but a complex type for 'returns'? Sadly, yes. We have existing commands that do just that. I already documented that new commands should avoid it (it's not extensible). >=20 > The QAPI schema's 'returns' becomes "return" on the wire. We suck. We could search-and-replace the schema, but why bother. Yeah, the discrepancy is a bit annoying; on the other hand, it makes it easy to tell schema apart from on-the-wire samples, at least for commands that return something :) >=20 > qmp-spec.txt is *wrong*! We actually use json-array in addition to > json-object. Yep, added to my list of doc improvements for v5. >> +++ b/tests/qapi-schema/returns-unknown.out >> @@ -0,0 +1,3 @@ >> +[OrderedDict([('command', 'oops'), ('returns', 'NoSuchType')])] >> +[] >> +[] >=20 > scripts/qapi* is a sick joke when it comes to semantic analysis. That gets a lot better in 13/19 :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --C9RttlS26RpbAUjAn9nj62HI1vdpRF6iG 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 iQEcBAEBCAAGBQJUJB6zAAoJEKeha0olJ0NqPVAH+gO9ExHr2xWC0SSKDVn4qAK+ d/KaMuTTFhskIfimwsrJYIDRkxaOjwkU0Bo4q8Pll/8ipy0IfSamYOuJVlgmU084 fixrOGwV8NuiFfqqhd+ABCk70Q4AqJf9h8e7a878iO6wOJWp41IEk9n7y23eV0HL O33PBGqtRTPVd8Py3Jdnbx8QFLKONx6d2s8WfjRCi3vn+jthvKrEVQ7iMjGsuK2C oJUgE5TpKz3p5r/1B+i2qJlmBvAMDB+UhuAMwnCW3wBuqPrNVVsh4BziKa/VuDnD lS7xbxM+0NEYznuLA36E/0ZUENS/wE5pyBoWfv8oRZqC+eeQWQ0xAYOYQ7eRF+w= =obTL -----END PGP SIGNATURE----- --C9RttlS26RpbAUjAn9nj62HI1vdpRF6iG--