From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59570) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avnDv-0004ME-Jm for qemu-devel@nongnu.org; Thu, 28 Apr 2016 10:47:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avnDr-0004v4-CJ for qemu-devel@nongnu.org; Thu, 28 Apr 2016 10:47:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36321) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avnDr-0004v0-4t for qemu-devel@nongnu.org; Thu, 28 Apr 2016 10:47:19 -0400 References: <1461801715-24307-1-git-send-email-eblake@redhat.com> <1461801715-24307-9-git-send-email-eblake@redhat.com> <87d1p9n7ul.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <57222274.1090207@redhat.com> Date: Thu, 28 Apr 2016 08:47:16 -0600 MIME-Version: 1.0 In-Reply-To: <87d1p9n7ul.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="w8Bwl6wtwswJDEGipfGsAOTXW8TAb2k3u" Subject: Re: [Qemu-devel] [PATCH v15 08/23] monitor: Let generated code validate arguments List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth , Luiz Capitulino , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --w8Bwl6wtwswJDEGipfGsAOTXW8TAb2k3u Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/28/2016 08:09 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Having to manually call out the set of expected arguments in >> qmp-commands.hx, in addition to what is already in *.json, >> is tedious and prone to error. The only reason we use >> .args_type is for checking if there is any excess arguments >> or incorrectly typed arguments during qmp_check_client_args(), >> but a strict QMP Input visitor also does that checking. >=20 > We also use it for completion. Does scripts/qmp/qmp-shell do completion? It's a script, is it parsing the .hx file? I know we do completion for HMP, but this is QMP that I'm tweaking, and other than qmp-shell, I don't know of any qemu code that wants to do QMP completion. We have query-qmp-schema that could be used to provide completion, if needed. >> and for commands with at least one (possibly-optional) argument, >> the output changes from: >> >> {"execute":"query-command-line-options","arguments":{"a":1}} >> {"error": {"class": "GenericError", "desc": "Invalid parameter 'a'"}} >> QERR_INVALID_PARAMETER >> to: >> >> {"execute":"query-command-line-options","arguments":{"a":1}} >> {"error": {"class": "GenericError", "desc": "QMP input object member '= a' is unexpected"}} >=20 > This error message becomes rather generic. Tolerable. Can we restore > the old message without trouble? QERR_QMP_EXTRA_MEMBER Yes, it's quite easy to switch between the two error message strings by tweaking the choice of message in qmp_input_check_struct() (at the end of the series; it's in qmp_input_pop() at this point of the series). >> >> - (void)args; >> -''') >> + if (args && qdict_size(args)) { >> + error_setg(errp, "Command '%%s' does not take arguments", "%(= name)s"); >> + return; >> + } >> +''', >> + name=3Dname) >> >> ret +=3D gen_call(name, arg_type, ret_type) >> >=20 > Works for me. >=20 > Naive question: is the special case "no arguments" really necessary > here? Could we visit the empty struct instead? If we had an empty struct visitor around. But right now the generator special-cases ':empty' so that it has no generated functions. >> @@ -2362,7 +2281,7 @@ EQMP >> >> { >> .name =3D "query-qmp-schema", >> - .args_type =3D "", >> + .args_type =3D "", >> .mhandler.cmd_new =3D qmp_query_qmp_schema, >> }, >=20 > Spurious hunk. Whoops. I first deleted it, then realized this was one of the few places that doesn't use qmp_marshal_ so I had to restore it, but restored it with the wrong whitespace. >=20 > Entries defining anything beyond .name and .mhandler.cmd_new: >=20 > * device_add, qmp_capabilities >=20 > Not QAPIfied, need everything. >=20 > * netdev_add, query-qmp-schema >=20 > Need .args_type because of 'gen': false. >=20 > * client_migrate_info, dump-guest-memory, query-dump, getfd, closefd, > add-fd, remove-fd, query-fdsets, migrate-set-capabilities >=20 > Define .params and/or .help. Does this make any sense? >=20 > A comment explaining which members need to be set would be nice. >=20 > Have you checked Marc-Andr=C3=A9's work for overlap? Cc'ing him. Marc-Andr=C3=A9 QAPIfies device_add and qmp_capabilities, then completely= eliminates qmp-commands.hx. This probably duplicates some of the work in his queue, but should be fine in the short term. As for whether any of the other fields are needed or useful, I didn't check (but suspect that no, they are not needed, again because this is QMP not HMP and that only HMP 'info cmd' cares). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --w8Bwl6wtwswJDEGipfGsAOTXW8TAb2k3u 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/ iQEcBAEBCAAGBQJXIiJ0AAoJEKeha0olJ0NqYEQH/jY1SpcUMGfiaLi/tlJhey2n fcN4u6Mroi9gy2WpZz8fbkqaRucQURWR8hxQydVA+Hnbr2JY/ovKTgUcter16jLd Z56qEsKopMnUeSRhOxIxNcWJMqbpvSZpWPuiqXdsXs5EriSZ0jq4p8+FPpaS9wnb Gt/d1385zkpIgseKPbHh329R7XwyNZdZUNu+W1eD8TQB/oSbT1vjDKui5R1YKYnn DTPUNmNlacS55mwQex9MMxwd6SH97YwqY62l4l8uPgt/Zj1ADsKv3U4b5C6S1nsM odyY4jECO0GaWwt1fUqdWVcj6x2TBLxUq2LEm7evcdgWCJn2AvSmnxPbew2Kbz8= =Nsjv -----END PGP SIGNATURE----- --w8Bwl6wtwswJDEGipfGsAOTXW8TAb2k3u--