From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHuMY-0005qW-CX for qemu-devel@nongnu.org; Thu, 14 Aug 2014 08:42:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XHuMT-0001zb-Mg for qemu-devel@nongnu.org; Thu, 14 Aug 2014 08:42:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHuMT-0001yj-EZ for qemu-devel@nongnu.org; Thu, 14 Aug 2014 08:42:33 -0400 Message-ID: <53ECAEB6.5070602@redhat.com> Date: Thu, 14 Aug 2014 06:42:30 -0600 From: Eric Blake MIME-Version: 1.0 References: <1407287673-20308-1-git-send-email-eblake@redhat.com> <1407287673-20308-10-git-send-email-eblake@redhat.com> <87ppg3idot.fsf@blackfin.pond.sub.org> In-Reply-To: <87ppg3idot.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="jlInOarqWV4hrSX2QeoNqb2sT9QuUdGAm" Subject: Re: [Qemu-devel] [PATCH v3 09/14] qapi: add check_type helper function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Luiz Capitulino , Fam Zheng , qemu-devel@nongnu.org, wenchaoqemu@gmail.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --jlInOarqWV4hrSX2QeoNqb2sT9QuUdGAm Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/14/2014 04:10 AM, Markus Armbruster wrote: > Subject suggests you're merely adding a helper function. You're > actually fixing bugs. Something like >=20 > qapi: More rigorous checking of type expressions >=20 > would be clearer. Along with squashing in 8/14, you are correct about this fixing bugs (serves me right for refactoring, then realizing that we needed more tests, then fixing the tests without checking that the subject line stayed appropriate). >> +++ b/scripts/qapi.py >> @@ -329,6 +329,32 @@ def check_union(expr, expr_info): >> # Todo: add checking for values. Key is checked as above, val= ue can be >> # also checked here, but we need more functions to handle arr= ay case. >> >> +def check_type(expr_info, name, data, allow_native =3D False): >> + if not data: >> + return >> + if isinstance(data, list): >> + if len(data) !=3D 1 or not isinstance(data[0], basestring): >> + raise QAPIExprError(expr_info, >> + "Use of array type in '%s' must conta= in " >> + "single element type name" % name) >> + data =3D data[0] >=20 > Peeling off the array here means error messages below won't mention it.= > Visible in data-array-unknown.err below. But good enough is good > enough. >=20 >> + >> + if isinstance(data, basestring): >> + if find_struct(data) or find_enum(data) or find_union(data): >> + return >> + if data =3D=3D 'str' or data =3D=3D 'int' or data =3D=3D 'vis= itor': >=20 > Is this complete? Should we be checking against builtin_types[]? It was complete insofar that it passes with the current qapi-schema.json. Checking builtin_types[] would indeed be nicer (I didn't know that existed). >=20 > Pardon my ignorance: where does 'visitor' come from? qom-set, qom-get. Nasty commands that aren't typesafe, and which explicitly use 'gen':'no' to abuse the qapi system by bypassing the code generator while still exposing a backdoor command that can break the rule= s. netdev_add ('*props':'**') and object-add ('*props':'dict') are the only other 'gen':'no' clients; I'm not sure why they didn't cause the parser to barf the way 'visitor' did. Maybe my approach should instead be to unify all four 'gen':'no' expressions to use the same syntax of "**" to represent the fact that the QMP code is not type-safe, as well as actually documenting this (ab)use of ** (the fact that none of the documentation mentions 'gen' is telling). Looks like I've just added a pre-req patch into my v4 :) >=20 >> + if allow_native: >> + return >> + raise QAPIExprError(expr_info, >> + "Primitive type '%s' not allowed as d= ata " >> + "of '%s'" % (data, name)) >> + raise QAPIExprError(expr_info, >> + "Unknown type '%s' as data of '%s'" >> + % (data, name)) >=20 > "as data of" suggests the problem is in member 'data', even when it's > actually in member 'returns'. Visible in returns-unknown.err below. The function is declared as: >> +def check_type(expr_info, name, data, allow_native =3D False): but allow_native is True only for the 'returns' case. Maybe I should just repurpose that parameter for what it really is - 'data' vs. 'returns', and use it for improving the error message in addition to deciding whether native types are allowed. >=20 >> + elif not isinstance(data, OrderedDict): >> + raise QAPIExprError(expr_info, >> + "Expecting dictionary for data of '%s'" %= name) >> + >> def check_exprs(schema): >> for expr_elem in schema.exprs: >> expr =3D expr_elem['expr'] >> @@ -356,6 +382,10 @@ def check_exprs(schema): >> raise QAPIExprError(info, >> "enum '%s' requires an array for = 'data'" >> % expr.get('enum')) >> + else: >=20 > This is for 'type' and 'command', right? 'type', 'union', 'event', and 'command' ('data' is a dict for those four, and an array for the fifth meta-type of 'enum'). I'll add a commen= t. >=20 >> + check_type(info, expr_name(expr), members) >> + if expr.has_key('command') and expr.has_key('returns'): >> + check_type(info, expr['command'], expr['returns'], Tr= ue) >> >> def parse_schema(input_file): >> try: >=20 > Looks pretty good, but I didn't check systematically against > qapi-code-gen.txt for correctness and completeness. We can always > improve on top. I'll double-check things as well, when posting v4. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --jlInOarqWV4hrSX2QeoNqb2sT9QuUdGAm 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 iQEcBAEBCAAGBQJT7K62AAoJEKeha0olJ0NqhEkH/1MKjbyu+Zr0EeVWzLnfHFTG LLWEAimxI2nStSj2QppCRHIbXEvo9cEY6l0kGjZRczEcdo96yEE9mW0QVybxXCWX TRTmpgR8lgTvCRccQXT/0veZnXdCN31nr7XOs+fYSz/n0oy0ImdUjbhK3ONY6JNK zClhQOfICZ6vAtyj/MMOlezyLFbNzRGAq/g3Hd5TzA7J1gqcM+PwIMfKZTzxkq/t Zi4eMu8DleWron1HG+BeZ3Yrw4Nc5k4Eb2gwZXbMvKi1CQPxKhR+bsZsjmA+a1Yc EiIAtlJanCohT84eN7XVufVWGXIju0j9l5G9//Cys/645/Gxm22j9zVgW1S688U= =XyLm -----END PGP SIGNATURE----- --jlInOarqWV4hrSX2QeoNqb2sT9QuUdGAm--