Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP

[Hitoshi Mitake]

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 <address@hidden>
> > Cc: Kevin Wolf <address@hidden>
> > Cc: Stefan Hajnoczi <address@hidden>
> > Signed-off-by: Hitoshi Mitake <address@hidden>
> > ---
> >  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