From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMTOJ-0006a6-GW for qemu-devel@nongnu.org; Tue, 26 Aug 2014 22:55:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMTOF-0007nS-Ks for qemu-devel@nongnu.org; Tue, 26 Aug 2014 22:55:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17046) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMTOF-0007n3-D7 for qemu-devel@nongnu.org; Tue, 26 Aug 2014 22:55:15 -0400 Message-ID: <53FD4881.4040007@redhat.com> Date: Tue, 26 Aug 2014 20:54:57 -0600 From: Eric Blake MIME-Version: 1.0 References: <1409104780-31445-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp> In-Reply-To: <1409104780-31445-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QL3WAD7jPP5mquI6duvthKGTFGgXaG0Tf" 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 , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , mitake.hitoshi@gmail.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --QL3WAD7jPP5mquI6duvthKGTFGgXaG0Tf Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/26/2014 07:59 PM, Hitoshi Mitake wrote: > This patch makes the fault injection functionality of blkdebug > callable from QMP. Motivation of this change is for testing and > debugging distributed systems. Ordinal distributed systems must handle > hardware faults because of its reason for existence, but testing > whether the systems can hanle such faults and recover in a correct > manner is really hard. >=20 > Example of QMP sequence (via telnet localhost 4444): >=20 > { "execute": "qmp_capabilities" } > {"return": {}} >=20 > {"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. > 1, "once": 0, "state": 1}]}} # <- inject error to /dev/sda It looks like 'immediately', 'once', and 'state' are all bools - if so, they should be typed as bool and the example should be written with "immediately":true and so forth. >=20 > {"return": {}} >=20 > Now the guest OS on the VM finds the disk is broken. >=20 > Of course, using QMP directly is painful for users (developers and > admins of distributed systems). I'm implementing user friendly > interface in vagrant-kvm [4] for blackbox testing. In addition, a > testing framework for injecting faults at critical timing (which > requires solid understanding of target systems) is in progress. >=20 > [1] http://ucare.cs.uchicago.edu/pdf/socc13-limplock.pdf > [2] http://docs.openstack.org/developer/swift/howto_installmultinode.ht= ml > [3] http://www.amazon.com/dp/B00C93QFHI > [4] https://github.com/adrahon/vagrant-kvm >=20 > Cc: Eric Blake > Cc: Kevin Wolf > Cc: Stefan Hajnoczi > Signed-off-by: Hitoshi Mitake > --- > block/blkdebug.c | 199 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > include/block/block.h | 2 + > qapi-schema.json | 14 ++++ > qmp-commands.hx | 18 +++++ > 4 files changed, 233 insertions(+) >=20 > + > +static void rules_list_iter(QObject *obj, void *opaque) > +{ > + struct qmp_rules_list_iter *iter =3D (struct qmp_rules_list_iter *= )opaque; This is C, not C++. The cast is spurious. > + } 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? > + _errno =3D qdict_get_try_int(dict, "errno", EIO); > + if (qemu_opt_set_number(new_opts, "errno", _errno) < 0) { > + error_setg(&iter->err, "faild to set errno"); s/faild/failed/ (multiple times throughout your patch) > + > + 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. > +++ b/qapi-schema.json > @@ -3481,3 +3481,17 @@ > # Since: 2.1 > ## > { 'command': 'rtc-reset-reinjection' } > + > +## > +# @blockdebug-set-rules > +# > +# Set rules of blkdebug for the given block device. > +# > +# @device: device ID of target block device > +# @rules: rules for setting, list of dictionary > +# > +# Since: 2.2 > +## > +{ 'command': 'blkdebug-set-rules', > + 'data': { 'device': 'str', 'rules': [ 'dict' ] }, > + 'gen': 'no'} Are we really forced to use this non-type-safe variant? I'd REALLY love to fully specify this interface in QAPI rather than just hand-waving that an undocumented dictionary is in place. Given your example, it looks like you'd want 'rules':['BlkdebugRule'], where you then have something like: { 'enum':'BlkdebugRuleEvent', 'data':['write-aio', ...] } { 'enum':'BlkdebugRuleType', 'data':['inject-error', ...] } { 'type':'BlkdebugRule', 'arguments':{ 'event':'BlkdebugRuleEvent', 'type':'BlkdebugRuleType', '*immediately':'bool', '*once':'bool', '*state':'bool' } } --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --QL3WAD7jPP5mquI6duvthKGTFGgXaG0Tf 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/UiBAAoJEKeha0olJ0Nq9jEIAJjftkfFzr7cHzFrM5jKB+LJ +vJXrRZztD/R6z1kxEmF56haQvK4tyJvn12GqarpgLBdPyevBy4q/qi9kF/xjgtA Uq50eWj3crIB42ZB1hbmNxMlbwRODzn4xO4Hcl7QldC1smK0i3WL8Q+VVoxU6RqN Zz9GiL0cvDUBf/TE4PlCHIzwhZdAswMCtPQch/1OZKDIki5RMIkFyFXVJdUHdPnJ ZomPdWYab2AF+K1dDHxMd/bkirU91gSmgGpswEGbINyJGSFn7AwqWIgYL2bNLyRb sMAk3WnKguMYRkNxeSZbw7xaDiaDsCBuq7jbIQELU0YGBWYZd59hWRn3tz3P9Ew= =inuq -----END PGP SIGNATURE----- --QL3WAD7jPP5mquI6duvthKGTFGgXaG0Tf--