From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMca1-0006ba-3x for qemu-devel@nongnu.org; Wed, 27 Aug 2014 08:44:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMcZw-0007Fm-3G for qemu-devel@nongnu.org; Wed, 27 Aug 2014 08:44:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMcZv-0007Fg-S6 for qemu-devel@nongnu.org; Wed, 27 Aug 2014 08:43:56 -0400 Message-ID: <53FDD285.4090406@redhat.com> Date: Wed, 27 Aug 2014 06:43:49 -0600 From: Eric Blake MIME-Version: 1.0 References: <1409104780-31445-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp> <53FD4881.4040007@redhat.com> <87r402binc.wl%mitake.hitoshi@lab.ntt.co.jp> In-Reply-To: <87r402binc.wl%mitake.hitoshi@lab.ntt.co.jp> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GPP9bck5AQoTej1RAvaQKqU4V1nJ6pCtf" Subject: Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hitoshi Mitake Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, mitake.hitoshi@gmail.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --GPP9bck5AQoTej1RAvaQKqU4V1nJ6pCtf Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/26/2014 11:34 PM, Hitoshi Mitake wrote: >>> {"execute": "blkdebug-set-rules", "arguments": {"device": "ide0-hd0",= >>> "rules":[{"event": "write_aio", "type": "inject-error", "immediately"= : >> >> Why 'write_aoi'? New QMP commands should prefer dashes (write-aio) if >> there is no compelling reason for underscore. >=20 > The name of event is defined in the event_names array of > block/blkdebug.c, which uses underscore. I think using the original > names would be good because the name shouldn't be different from > config file notation of blkdebug. But it should be fairly easy to code things to accept both spellings when parsing the config file, while preferring the dash in QAPI. Furthermore, by defining the enum in QAPI instead of in blkdebug.c, you get a generated enum that blkdebug.c can reuse, and where you are then guaranteed that things stay in sync if new commands are added (by adding them to the .json file). >> >>> + } else if (!strcmp(type, "inject-error")) { >>> + int _errno, sector; >> >> The name _errno threw me; is there something better without a leading >> underscore that would work better? >=20 > I added underscore to the name because errno is already used by > errno.h. How about "given_errno"? If you use proper .json typing, the generator will automatically rename the QMP interface "errno" to the C code "q_errno". Consistently using the type-safe munging already done by the generator, instead of redoing a completely different munging on your own, is the preferred solution her= e. >>> + >>> + once =3D qdict_get_try_bool(dict, "once", 0); >> >> s/0/bool/ - we use , so you should use the named constants >> when dealing with bool parameters. >=20 > Thanks, I'll fix it in v3. Actually, if you use a type-safe qapi definition, you may not even need to do raw qdict operations, but can just directly use the C struct that gets generated as a result of the qapi. >=20 > I also like the detailed specification. But, there are bunch of event > names (the event_names array of block/blkdebug.c). In addition, the > rule of blkdebug can be extended. So I think defining it in two palces > (qapi-schema.json and block/blkdebug.c) is hard to maintain. Parsing > qdict in blkdebug.c would be simpler. How do you think? No, don't do duplication. Instead, fix blkdebug.c to USE the enum generated by the .json, and you only have to maintain the list in one place - the .json file. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --GPP9bck5AQoTej1RAvaQKqU4V1nJ6pCtf 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 iQEcBAEBCAAGBQJT/dKFAAoJEKeha0olJ0NqKIQIAKv2j+WBYld3pn/HY7BB+ZDs DQKk3Xa27DgUkDfh6XENzqgXCJyyz0MNfCnO/qe/KijEztg0dDqp+pgmq5cG041R PYpCguGmBcXqD55DQymPUqPp9V9OpiBqk1Cfyh9n1RloaMkLBTdUwiKjVFxHMelR eMRAqHe2zEMTV4Ls8AVk8idA5iYoUOvFRv4ozqDCw5wnBlGhE911Ldl+ed8PVuL5 Tfv4BZssLbOCxvvEDi6Oc9ydEMsmrbBsTniNRDkq/dRfYBjJ5YDOgmliFGoNHsE5 xkCXS4YSh+BqAcdB6I6XGV8TW7lVpOrpha18rtGyYkKqikNP+E7l67L5Eauahlo= =ZV7P -----END PGP SIGNATURE----- --GPP9bck5AQoTej1RAvaQKqU4V1nJ6pCtf--