From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59925) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1av2XW-0000ID-D9 for qemu-devel@nongnu.org; Tue, 26 Apr 2016 08:56:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1av2XS-0005nq-4P for qemu-devel@nongnu.org; Tue, 26 Apr 2016 08:56:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41681) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1av2XR-0005nm-SM for qemu-devel@nongnu.org; Tue, 26 Apr 2016 08:56:26 -0400 References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-16-git-send-email-eblake@redhat.com> <87shynt7xx.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <571F6577.7050801@redhat.com> Date: Tue, 26 Apr 2016 06:56:23 -0600 MIME-Version: 1.0 In-Reply-To: <87shynt7xx.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="aRqEKClDIL0XGHR08XtPi4cn69DmfIWIq" Subject: Re: [Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in visit_start_struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --aRqEKClDIL0XGHR08XtPi4cn69DmfIWIq Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/15/2016 05:42 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The qmp-input visitor was playing rather fast and loose: when >=20 > I guess (some of) its *users* are playing fast and loose, and the > visitor itself lets them. The patch's title suggests "some of its > users" =3D=3D qapi-commands.py. >=20 >> visiting a QDict, you could grab members of the root dictionary >> without first pushing into the dict. But we are about to tighten >> the input visitor, at which point the generated marshal code >> MUST follow the same paradigms as everyone else, of pushing into >> the struct before grabbing its keys, because the value of 'name' >> should be ignored on the top-level visit. >> >> Generated code grows as follows: >> >> |@@ -515,7 +695,15 @@ void qmp_marshal_blockdev_backup(QDict * >> | BlockdevBackup arg =3D {0}; >> | >> | v =3D qmp_input_get_visitor(qiv); >> |+ visit_start_struct(v, NULL, NULL, 0, &err); >> |+ if (err) { >> |+ goto out; >> |+ } >> | visit_type_BlockdevBackup_members(v, &arg, &err); >> |+ if (!err) { >> |+ visit_check_struct(v, &err); >> |+ } >=20 > Does this find errors that weren't found before? All that this could find is excess input, but we are already checking for excess input prior to calling into the generated marshaling code, so it doesn't find anything new. >> >> Note that this change could also make it possible for the >> marshalling code to automatically detect excess input at the top >> level, and not just in nested dictionaries. However, that checking >> is not currently useful (and we rely on the manual checking in >> monitor.c:qmp_check_client_args() instead) as long as qmp-commands.hx >> uses .args_type, and as long as we have 'name:O' as an arg-type that >> explicitly allows unknown top-level keys because we haven't yet >> converted 'device_add' and 'netdev_add' to introspectible use of >> 'any'. >=20 > Hmm, that explains why finding these additional errors wouldn't be > useful. Good to know, but doesn't quite answer my question. I guess what it really translates to is we are now doing redundant checking, and I should do a followup patch to simplify monitor.c. >> @@ -150,7 +158,9 @@ out: >> qmp_input_visitor_cleanup(qiv); >> qdv =3D qapi_dealloc_visitor_new(); >> v =3D qapi_dealloc_get_visitor(qdv); >> + visit_start_struct(v, NULL, NULL, 0, NULL); >> visit_type_%(c_name)s_members(v, &arg, NULL); >> + visit_end_struct(v); >> qapi_dealloc_visitor_cleanup(qdv); >> ''', >> c_name=3Darg_type.c_name()) >=20 > No visit_check_struct() here. I think its contract should explicitly > state that you may omit it when you're not interested in errors. Indeed, calling visit_check_struct(, NULL) can't report any errors, so skipping it should be documented as safe. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --aRqEKClDIL0XGHR08XtPi4cn69DmfIWIq Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXH2V3AAoJEKeha0olJ0Nq9+EIAJ7U8ba7DCgIuvRxE1hnB3O1 82Fv/yU6aULvnTwoMM9CPjKn2nI+GJ3gi3yNwbYCtJ+Myy6Iw4+tYsXtosDBXeDW iMO7ev6cCOgaK2AiHzRzykGLGMDUhnBNlP36aw6UwRkBbKrFeyA3EEs7blD/NO6r iUW1hrWLRmwynkvmSq+Av3/v813qoBRwRWmF3whPXSuAprnm0He7kOmoNUPX8wiy Qttuxa21tyg/B8/yjRyDgvoLg40OVQ4nZjCndILokwspT0OJDbegtYdjud/XnwTM slGfcIs18h+iSIs1uc41E0fRJ0DVTNTXfZJAShxJAwGhbdrjBEty+In0+cQzx7Q= =siHP -----END PGP SIGNATURE----- --aRqEKClDIL0XGHR08XtPi4cn69DmfIWIq--