All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
@ 2014-08-27  1:59 Hitoshi Mitake
  2014-08-27  2:54 ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Hitoshi Mitake @ 2014-08-27  1:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Hitoshi Mitake, Kevin Wolf, Stefan Hajnoczi, mitake.hitoshi

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.

Typically, developers of distributed systems check such recovery paths
with unit test or artificial environment which can be built in a
single box. But such tests can miss important attributes of real world
hardware faults. Examples of disk drive:
 - write(2) doesn't return -1 immediately in a case of disk error even
   a target file is opened with O_SYNC, if file system of the file is
   not mounted with barrier option
 - some disks become silent suddenly without causing errors, so
   applications must handle such a case with fine tuned timeout of
   disk I/O
 - some disks can cause performance degradation instead of stopping
   and causing errors [1]

For testing recovery paths and configuration of distributed systems,
mocking faults like the above examples in virtual devices is
effective. Because ordinal testing techniques which target errors of
library APIs and systemcalls cannot mock the above faults. In
addition, injecting faults at the level of virtual devices can test
whole stack of target systems (from device drivers to
applications). As a first step of implementing this testing technique,
this patch implements a new QMP command which updates error injection
rules of blkdebug. I think it is more useful for testing distributed
systems than existing config file based fault injection of
blkdebug. Because users can inject faults at any time.

With this feature, I could find a potential problem in the deployment
guide of OpenStack Swift [2]. In the guide, nobarrier option of xfs is
suggested without any caution. The option degrades durability of Swift
cluster because it delays detection of disk error. In addition, the
option is not suggested in a book of Swift guide [3]. So I concluded
the guide [2] can lead to a misconfiguration of Swift. I believe this
sort of problem can be found in other systems so the feature is useful
for developers and admins of distributed systems.

Example of launching QEMU with this feature:

sudo x86_64-softmmu/qemu-system-x86_64 -qmp \
tcp:localhost:4444,server,nowait -enable-kvm -hda \
blkdebug:/dev/null:/tmp/debian.qcow2

(/dev/null is needed because blkdebug requires configuration file, but
for QMP purpose empty file is enough)

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":
1, "once": 0, "state": 1}]}} # <- inject error to /dev/sda

{"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(+)

v2:
 - don't prepare a new mechanism for fault injection
 -- implement the feature with updating fault rules of blkdebug
 - add an example of QMP command

diff --git a/block/blkdebug.c b/block/blkdebug.c
index f51407d..2b9d616 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -687,6 +687,205 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
     return bdrv_getlength(bs->file);
 }
 
+struct qmp_rules_list_iter {
+    bool failed;
+    QemuOpts *set_state, *inject_error;
+
+    Error *err;
+};
+
+static void rules_list_iter(QObject *obj, void *opaque)
+{
+    struct qmp_rules_list_iter *iter = (struct qmp_rules_list_iter *)opaque;
+    QemuOpts *new_opts;
+    QDict *dict;
+    Error *err;
+    const char *type;
+
+    const char *event_name;
+    int state;
+
+    if (iter->failed) {
+        /* do nothing anymore */
+        return;
+    }
+
+    dict = qobject_to_qdict(obj);
+    if (!dict) {
+        error_set(&iter->err, QERR_INVALID_PARAMETER_TYPE,
+                  "member of rules", "dict");
+        goto fail;
+    }
+
+    event_name = qdict_get_str(dict, "event");
+    if (!event_name) {
+        error_set(&iter->err, QERR_MISSING_PARAMETER, "event");
+        goto fail;
+    }
+
+    state = qdict_get_try_int(dict, "state", 0);
+
+    type = qdict_get_str(dict, "type");
+    if (!strcmp(type, "set-state")) {
+        int new_state;
+
+        if (iter->set_state) {
+            error_setg(&iter->err, "duplicate entry for set-state");
+            goto fail;
+        }
+
+        new_opts = qemu_opts_create(&set_state_opts, NULL, 0, &err);
+        if (!new_opts) {
+            iter->err = err;
+            goto fail;
+        }
+
+        iter->set_state = new_opts;
+
+        new_state = qdict_get_try_int(dict, "new_state", 0);
+        if (qemu_opt_set_number(new_opts, "new_state", new_state) < 0) {
+            error_setg(&iter->err, "faild to set new_state");
+            goto fail;
+        }
+    } else if (!strcmp(type, "inject-error")) {
+        int _errno, sector;
+        bool once, immediately;
+
+        if (iter->inject_error) {
+            error_setg(&iter->err, "duplicate entry for inject-error");
+            goto fail;
+        }
+
+        new_opts = qemu_opts_create(&inject_error_opts, NULL, 0, &err);
+        if (!new_opts) {
+            iter->err = err;
+            goto fail;
+        }
+
+        iter->inject_error = new_opts;
+
+        _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");
+            goto fail;
+        }
+
+        sector = qdict_get_try_int(dict, "sector", -1);
+        if (qemu_opt_set_number(new_opts, "sector", sector) < 0) {
+            error_setg(&iter->err, "faild to set sector");
+            goto fail;
+        }
+
+        once = qdict_get_try_bool(dict, "once", 0);
+        if (qemu_opt_set_bool(new_opts, "once", once) < 0) {
+            error_setg(&iter->err, "faild to set once");
+            goto fail;
+        }
+
+        immediately = qdict_get_try_bool(dict, "immediately", 0);
+        if (qemu_opt_set_bool(new_opts, "immediately", immediately) < 0) {
+            error_setg(&iter->err, "faild to set immediately");
+            goto fail;
+        }
+    } else {
+        error_setg(&iter->err, "unknown type of rule: %s", type);
+        goto fail;
+    }
+
+    if (qemu_opt_set_number(new_opts, "state", state) < 0) {
+        error_setg(&iter->err, "faild to set state");
+        goto fail;
+    }
+
+    if (qemu_opt_set(new_opts, "event", event_name) < 0) {
+        error_setg(&iter->err, "faild to set event");
+        goto fail;
+    }
+
+    return;
+
+fail:
+    iter->failed = true;
+}
+
+int qmp_blkdebug_set_rules(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    QObject *rules = qdict_get(qdict, "rules");
+    const QList *rules_list = NULL;
+    Error *local_err = NULL;
+    BlockDriverState *bs;
+    BDRVBlkdebugState *s;
+    struct qmp_rules_list_iter iter;
+    struct add_rule_data d;
+
+    if (!device) {
+        error_set(&local_err, QERR_MISSING_PARAMETER, "device");
+        goto out;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, device);
+        goto out;
+    }
+
+    bs = bs->file;
+    if (strcmp(bs->drv->format_name, "blkdebug")) {
+        error_setg(&local_err, "BlockDriver (%s) isn't blkdebug",
+                   bs->drv->format_name);
+        goto out;
+    }
+    s = bs->opaque;
+
+    if (!rules) {
+        error_set(&local_err, QERR_MISSING_PARAMETER, "rules");
+        goto out;
+    }
+
+    rules_list = qobject_to_qlist(rules);
+    if (!rules_list) {
+        error_set(&local_err, QERR_INVALID_PARAMETER_TYPE, "rules", "list");
+        goto out;
+    }
+
+    memset(&iter, 0, sizeof(iter));
+    qlist_iter(rules_list, rules_list_iter, &iter);
+    if (iter.failed) {
+        local_err = iter.err;
+        goto out;
+    }
+
+    d.s = s;
+    s->state = 1;
+    if (iter.inject_error) {
+        d.action = ACTION_INJECT_ERROR;
+        add_rule(iter.inject_error, &d);
+    }
+
+    if (iter.set_state) {
+        d.action = ACTION_SET_STATE;
+        add_rule(iter.set_state, &d);
+    }
+
+out:
+    if (iter.inject_error) {
+        qemu_opts_del(iter.inject_error);
+    }
+
+    if (iter.set_state) {
+        qemu_opts_del(iter.set_state);
+    }
+
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
+}
+
 static BlockDriver bdrv_blkdebug = {
     .format_name            = "blkdebug",
     .protocol_name          = "blkdebug",
diff --git a/include/block/block.h b/include/block/block.h
index f08471d..421a1b5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -588,4 +588,6 @@ void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 void bdrv_flush_io_queue(BlockDriverState *bs);
 
+int qmp_blkdebug_set_rules(Monitor *mon, const QDict *qdict, QObject **ret);
+
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 341f417..13bab1d 100644
--- a/qapi-schema.json
+++ 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'}
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4be4765..ef42cf0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3755,3 +3755,21 @@ Example:
 <- { "return": {} }
 
 EQMP
+    {
+        .name       = "blkdebug-set-rules",
+        .args_type  = "device:s,rules:q",
+        .mhandler.cmd_new = qmp_blkdebug_set_rules,
+    },
+SQMP
+blkdebug-set-rules
+------------------
+
+Set blockdebug rules
+
+Example:
+-> {"execute": "blkdebug-set-rules", "arguments": {"device":
+   "ide0-hd0", "rules":[{"event": "write_aio", "type": "inject-error",
+   "immediately": 1, "once": 0, "state": 1}]}}
+<- { "return": {} }
+
+EQMP
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-08-27  1:59 [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP Hitoshi Mitake
@ 2014-08-27  2:54 ` Eric Blake
  2014-08-27  5:34   ` Hitoshi Mitake
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2014-08-27  2:54 UTC (permalink / raw)
  To: Hitoshi Mitake, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, mitake.hitoshi

[-- Attachment #1: Type: text/plain, Size: 4137 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-08-27  2:54 ` Eric Blake
@ 2014-08-27  5:34   ` Hitoshi Mitake
  2014-08-27 12:43     ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Hitoshi Mitake @ 2014-08-27  5:34 UTC (permalink / raw)
  To: Eric Blake
  Cc: Hitoshi Mitake, Kevin Wolf, Stefan Hajnoczi, qemu-devel, mitake.hitoshi

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

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-08-27  5:34   ` Hitoshi Mitake
@ 2014-08-27 12:43     ` Eric Blake
  2014-08-28  6:18       ` Hitoshi Mitake
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2014-08-27 12:43 UTC (permalink / raw)
  To: Hitoshi Mitake; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, mitake.hitoshi

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

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


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

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.


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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-08-27 12:43     ` Eric Blake
@ 2014-08-28  6:18       ` Hitoshi Mitake
  2014-09-02 15:29         ` Stefan Hajnoczi
  0 siblings, 1 reply; 7+ messages in thread
From: Hitoshi Mitake @ 2014-08-28  6:18 UTC (permalink / raw)
  To: Eric Blake
  Cc: Hitoshi Mitake, Kevin Wolf, Stefan Hajnoczi, qemu-devel, mitake.hitoshi

At Wed, 27 Aug 2014 06:43:49 -0600,
Eric Blake wrote:
> 
> [1  <text/plain; UTF-8 (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.
> > 
> > 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?
> > 
> > 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 here.
> 
> 
> >>> +
> >>> +        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.
> 
> 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.
> 
> 
> > 
> > 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.
> 

OK, I understand your opinion. But it requires amount of change in
existing blkdebug implementation. So I want to hear opinions from the
maintainers. How do you think about this idea (maintaining blkdebug
events in .json format for avoiding duplication), Stefan and Kevin?

Thanks,
Hitoshi

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-08-28  6:18       ` Hitoshi Mitake
@ 2014-09-02 15:29         ` Stefan Hajnoczi
  2014-09-11  2:50           ` Hitoshi Mitake
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2014-09-02 15:29 UTC (permalink / raw)
  To: Hitoshi Mitake; +Cc: Kevin Wolf, qemu-devel, mitake.hitoshi

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On Thu, Aug 28, 2014 at 03:18:48PM +0900, Hitoshi Mitake wrote:
> At Wed, 27 Aug 2014 06:43:49 -0600,
> Eric Blake wrote:
> > 
> > [1  <text/plain; UTF-8 (quoted-printable)>]
> > On 08/26/2014 11:34 PM, Hitoshi Mitake wrote:
> > 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.
> > 
> > 
> > > 
> > > 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.
> > 
> 
> OK, I understand your opinion. But it requires amount of change in
> existing blkdebug implementation. So I want to hear opinions from the
> maintainers. How do you think about this idea (maintaining blkdebug
> events in .json format for avoiding duplication), Stefan and Kevin?

It would be nice to specify the rule syntax using qapi JSON.

I'm not sure if we can eliminate the QemuOptsList but it's probably
possible to avoid qdict or ini file parsing and instead use qapi
visitors to convert input to C structs.

It's worth a try.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP
  2014-09-02 15:29         ` Stefan Hajnoczi
@ 2014-09-11  2:50           ` Hitoshi Mitake
  0 siblings, 0 replies; 7+ messages in thread
From: Hitoshi Mitake @ 2014-09-11  2:50 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Hitoshi Mitake, Kevin Wolf, qemu-devel, mitake.hitoshi


Hi Stefan, Eric, Sorry for my late reply.

At Tue, 2 Sep 2014 16:29:14 +0100,
Stefan Hajnoczi wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Thu, Aug 28, 2014 at 03:18:48PM +0900, Hitoshi Mitake wrote:
> > At Wed, 27 Aug 2014 06:43:49 -0600,
> > Eric Blake wrote:
> > > 
> > > [1  <text/plain; UTF-8 (quoted-printable)>]
> > > On 08/26/2014 11:34 PM, Hitoshi Mitake wrote:
> > > 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.
> > > 
> > > 
> > > > 
> > > > 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.
> > > 
> > 
> > OK, I understand your opinion. But it requires amount of change in
> > existing blkdebug implementation. So I want to hear opinions from the
> > maintainers. How do you think about this idea (maintaining blkdebug
> > events in .json format for avoiding duplication), Stefan and Kevin?
> 
> It would be nice to specify the rule syntax using qapi JSON.
> 
> I'm not sure if we can eliminate the QemuOptsList but it's probably
> possible to avoid qdict or ini file parsing and instead use qapi
> visitors to convert input to C structs.
> 
> It's worth a try.

OK, I'll do it in v3.

Thanks,
Hitoshi

> 
> Stefan
> [2  <application/pgp-signature (7bit)>]
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-09-11  2:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-27  1:59 [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP Hitoshi Mitake
2014-08-27  2:54 ` Eric Blake
2014-08-27  5:34   ` Hitoshi Mitake
2014-08-27 12:43     ` Eric Blake
2014-08-28  6:18       ` Hitoshi Mitake
2014-09-02 15:29         ` Stefan Hajnoczi
2014-09-11  2:50           ` Hitoshi Mitake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.