At Tue, 26 Aug 2014 20:54:57 -0600, Eric Blake wrote: > > 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. > > > > > Example of QMP sequence (via telnet localhost 4444): > > > > { "execute": "qmp_capabilities" } > > {"return": {}} > > > > {"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.
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. > > > 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. state is integer typed. But as you say, immediately and once are boolean typed so I'll fix here in v3, thanks. > > > > > {"return": {}} > > > > Now the guest OS on the VM finds the disk is broken. > > > > 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. > > > > [1] http://ucare.cs.uchicago.edu/pdf/socc13-limplock.pdf > > [2] http://docs.openstack.org/developer/swift/howto_installmultinode.html > > [3] http://www.amazon.com/dp/B00C93QFHI > > [4] https://github.com/adrahon/vagrant-kvm > > > > Cc: Eric Blake <ebl...@redhat.com> > > Cc: Kevin Wolf <kw...@redhat.com> > > Cc: Stefan Hajnoczi <stefa...@redhat.com> > > Signed-off-by: Hitoshi Mitake <mitake.hito...@lab.ntt.co.jp> > > --- > > block/blkdebug.c | 199 > > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/block/block.h | 2 + > > qapi-schema.json | 14 ++++ > > qmp-commands.hx | 18 +++++ > > 4 files changed, 233 insertions(+) > > > > > + > > +static void rules_list_iter(QObject *obj, void *opaque) > > +{ > > + struct qmp_rules_list_iter *iter = (struct qmp_rules_list_iter > > *)opaque; > > This is C, not C++. The cast is spurious. I'll remove it in v3. > > > > + } 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? I added underscore to the name because errno is already used by errno.h. How about "given_errno"? > > > > + _errno = 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) Sorry for that. I'll fix them in v3. > > > > + > > + once = qdict_get_try_bool(dict, "once", 0); > > s/0/bool/ - we use <stdbool.h>, so you should use the named constants > when dealing with bool parameters. Thanks, I'll fix it in v3. > > > > +++ 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' } } 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? Thanks, Hitoshi