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.

> 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.

> 
> {"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 <eblake@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@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.


> +    } 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 = 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 = 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.


> +++ 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' } }

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org