Patchwork [RFC,6/7] blkdebug: Add events and rules

login
register
mail settings
Submitter Kevin Wolf
Date March 15, 2010, 5:08 p.m.
Message ID <1268672915-12233-7-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/47768/
State New
Headers show

Comments

Kevin Wolf - March 15, 2010, 5:08 p.m.
Block drivers can trigger a blkdebug event whenever they reach a place where it
could be useful to inject an error for testing/debugging purposes.

Rules are read from a blkdebug config file and describe which action is taken
when an event is triggered. For now this is only injecting an error (with a few
options) or changing the state (which is an integer). Rules can be declared to
be active only in a specific state; this way later rules can distiguish on
which path we came to trigger their event.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c          |   13 +++
 block.h          |    9 ++
 block/blkdebug.c |  241 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block_int.h      |    2 +
 4 files changed, 264 insertions(+), 1 deletions(-)
Christoph Hellwig - March 28, 2010, 1:12 p.m.
On Mon, Mar 15, 2010 at 06:08:34PM +0100, Kevin Wolf wrote:
> +    fprintf(stderr, "bdrv_debug_event: %d\n", event);

Is this supposed to be in the final version or a leftover debugging aid?

> +#define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)

Why not call bdrv_debug_event directly?

> +    config = strdup(filename);
> +    config[c - filename] = '\0';
> +    ret = read_config(s, config);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    filename = c + 1;
> +
> +    /* Open the backing file */
> +    ret = bdrv_file_open(&s->hd, filename, flags);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return 0;

Don't we need to free config somewhere?
Kevin Wolf - March 29, 2010, 8 a.m.
Am 28.03.2010 15:12, schrieb Christoph Hellwig:
> On Mon, Mar 15, 2010 at 06:08:34PM +0100, Kevin Wolf wrote:
>> +    fprintf(stderr, "bdrv_debug_event: %d\n", event);
> 
> Is this supposed to be in the final version or a leftover debugging aid?

It's not there in the final version (which I have already sent out, btw,
so you have reviewed an old version)

>> +#define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
> 
> Why not call bdrv_debug_event directly?

Originally I had intended to add a flag to ./configure to enable
blkdebug and #ifdef this out if it's not compiled in. Maybe we still
want to add that, but then it's not really much that you save.

>> +    config = strdup(filename);
>> +    config[c - filename] = '\0';
>> +    ret = read_config(s, config);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    filename = c + 1;
>> +
>> +    /* Open the backing file */
>> +    ret = bdrv_file_open(&s->hd, filename, flags);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    return 0;
> 
> Don't we need to free config somewhere?

Oops, this one is still there in the final version. I'll send another
version of the series.

Kevin

Patch

diff --git a/block.c b/block.c
index 31d1ba4..d4adbc3 100644
--- a/block.c
+++ b/block.c
@@ -1532,6 +1532,19 @@  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
     return drv->bdrv_load_vmstate(bs, buf, pos, size);
 }
 
+void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
+{
+    BlockDriver *drv = bs->drv;
+
+    fprintf(stderr, "bdrv_debug_event: %d\n", event);
+    if (!drv || !drv->bdrv_debug_event) {
+        return;
+    }
+
+    return drv->bdrv_debug_event(bs, event);
+
+}
+
 /**************************************************************/
 /* handling of snapshots */
 
diff --git a/block.h b/block.h
index edf5704..2fd8361 100644
--- a/block.h
+++ b/block.h
@@ -207,4 +207,13 @@  int bdrv_get_dirty(BlockDriverState *bs, int64_t sector);
 void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
                       int nr_sectors);
 int64_t bdrv_get_dirty_count(BlockDriverState *bs);
+
+
+typedef enum {
+    BLKDBG_EVENT_MAX,
+} BlkDebugEvent;
+
+#define BLKDBG_EVENT(bs, evt) bdrv_debug_event(bs, evt)
+void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event);
+
 #endif
diff --git a/block/blkdebug.c b/block/blkdebug.c
index f8ccd3c..22b1768 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -31,12 +31,15 @@ 
 typedef struct BDRVBlkdebugState {
     BlockDriverState *hd;
 
+    int state;
     int inject_errno;
     bool inject_once;
 
     /* Decides if aio_readv/writev fails right away (true) or returns an error
      * return value only in the callback (false) */
     bool inject_immediately;
+
+    QLIST_HEAD(list, blkdebug_rule) rules[BLKDBG_EVENT_MAX];
 } BDRVBlkdebugState;
 
 struct callback_data {
@@ -46,16 +49,210 @@  struct callback_data {
     void *opaque;
 };
 
+enum {
+    ACTION_INJECT_ERROR,
+    ACTION_SET_STATE,
+};
+
+struct blkdebug_rule {
+    BlkDebugEvent event;
+    int action;
+    int state;
+    union {
+        struct {
+            int error;
+            int immediately;
+            int once;
+        } inject;
+        struct {
+            int new_state;
+        } set_state;
+    } options;
+    QLIST_ENTRY(blkdebug_rule) next;
+};
+
+static QemuOptsList inject_error_opts = {
+    .name = "inject-error",
+    .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
+    .desc = {
+        {
+            .name = "event",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "state",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "errno",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "once",
+            .type = QEMU_OPT_BOOL,
+        },
+        {
+            .name = "immediately",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList set_state_opts = {
+    .name = "set-state",
+    .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
+    .desc = {
+        {
+            .name = "event",
+            .type = QEMU_OPT_STRING,
+        },
+        {
+            .name = "state",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
+            .name = "new_state",
+            .type = QEMU_OPT_NUMBER,
+        },
+        { /* end of list */ }
+    },
+};
+
+static QemuOptsList *config_groups[] = {
+    &inject_error_opts,
+    &set_state_opts,
+    NULL
+};
+
+static const char *event_names[BLKDBG_EVENT_MAX] = {
+};
+
+static int get_event_by_name(const char *name, BlkDebugEvent *event)
+{
+    int i;
+
+    for (i = 0; i < BLKDBG_EVENT_MAX; i++) {
+        if (!strcmp(event_names[i], name)) {
+            *event = i;
+            return 0;
+        }
+    }
+
+    return -1;
+}
+
+struct add_rule_data {
+    BDRVBlkdebugState *s;
+    int action;
+};
+
+static int add_rule(QemuOpts *opts, void *opaque)
+{
+    struct add_rule_data *d = opaque;
+    BDRVBlkdebugState *s = d->s;
+    const char* event_name;
+    BlkDebugEvent event;
+    struct blkdebug_rule *rule;
+
+    /* Find the right event for the rule */
+    event_name = qemu_opt_get(opts, "event");
+    if (!event_name || get_event_by_name(event_name, &event) < 0) {
+        return -1;
+    }
+
+    /* Set attributes common for all actions */
+    rule = qemu_mallocz(sizeof(*rule));
+    *rule = (struct blkdebug_rule) {
+        .event  = event,
+        .action = d->action,
+        .state  = qemu_opt_get_number(opts, "state", 0),
+    };
+
+    /* Parse action-specific options */
+    switch (d->action) {
+    case ACTION_INJECT_ERROR:
+        rule->options.inject.error = qemu_opt_get_number(opts, "errno", EIO);
+        rule->options.inject.once  = qemu_opt_get_bool(opts, "once", 0);
+        rule->options.inject.immediately =
+            qemu_opt_get_bool(opts, "immediately", 0);
+        break;
+
+    case ACTION_SET_STATE:
+        rule->options.set_state.new_state =
+            qemu_opt_get_number(opts, "new_state", 0);
+        break;
+    };
+
+    /* Add the rule */
+    QLIST_INSERT_HEAD(&s->rules[event], rule, next);
+
+    return 0;
+}
+
+static int read_config(BDRVBlkdebugState *s, const char *filename)
+{
+    FILE *f;
+    int ret;
+    struct add_rule_data d;
+
+    f= fopen(filename, "r");
+    if (f == NULL) {
+        return -errno;
+    }
+
+    ret = qemu_config_parse(f, config_groups);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    d.s = s;
+    d.action = ACTION_INJECT_ERROR;
+    qemu_opts_foreach(&inject_error_opts, add_rule, &d, 0);
+
+    d.action = ACTION_SET_STATE;
+    qemu_opts_foreach(&set_state_opts, add_rule, &d, 0);
+
+    ret = 0;
+fail:
+    fclose(f);
+    return ret;
+}
+
+/* Valid blkdebug filenames look like blkdebug:path/to/config:path/to/image */
 static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
+    int ret;
+    char *config, *c;
 
+    /* Parse the blkdebug: prefix */
     if (strncmp(filename, "blkdebug:", strlen("blkdebug:"))) {
         return -EINVAL;
     }
     filename += strlen("blkdebug:");
 
-    return bdrv_file_open(&s->hd, filename, flags);
+    /* Read rules from config file */
+    c = strchr(filename, ':');
+    if (c == NULL) {
+        return -EINVAL;
+    }
+
+    config = strdup(filename);
+    config[c - filename] = '\0';
+    ret = read_config(s, config);
+    if (ret < 0) {
+        return ret;
+    }
+    filename = c + 1;
+
+    /* Open the backing file */
+    ret = bdrv_file_open(&s->hd, filename, flags);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return 0;
 }
 
 static void error_callback_bh(void *opaque)
@@ -128,6 +325,7 @@  static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
 static void blkdebug_close(BlockDriverState *bs)
 {
     BDRVBlkdebugState *s = bs->opaque;
+    // FIXME Free data structures
     bdrv_delete(s->hd);
 }
 
@@ -144,6 +342,45 @@  static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
     return bdrv_aio_flush(s->hd, cb, opaque);
 }
 
+static void process_rule(BlockDriverState *bs, struct blkdebug_rule *rule)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+
+    /* Only process rules for the current state */
+    if (rule->state && rule->state != s->state) {
+        return;
+    }
+
+    /* Take the action */
+    switch (rule->action) {
+    case ACTION_INJECT_ERROR:
+        s->inject_errno         = rule->options.inject.error;
+        s->inject_once          = rule->options.inject.once;
+        s->inject_immediately   = rule->options.inject.immediately;
+        break;
+
+    case ACTION_SET_STATE:
+        // FIXME Need to use the old state to process more rules for this event
+        s->state                = rule->options.set_state.new_state;
+        break;
+    }
+}
+
+static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    struct blkdebug_rule *rule;
+
+    fprintf(stderr, "blkdebug_debug_event: %d\n", event);
+    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
+        return;
+    }
+
+    QLIST_FOREACH(rule, &s->rules[event], next) {
+        process_rule(bs, rule);
+    }
+}
+
 static BlockDriver bdrv_blkdebug = {
     .format_name        = "blkdebug",
     .protocol_name      = "blkdebug",
@@ -157,6 +394,8 @@  static BlockDriver bdrv_blkdebug = {
     .bdrv_aio_readv     = blkdebug_aio_readv,
     .bdrv_aio_writev    = blkdebug_aio_writev,
     .bdrv_aio_flush     = blkdebug_aio_flush,
+
+    .bdrv_debug_event   = blkdebug_debug_event,
 };
 
 static void bdrv_blkdebug_init(void)
diff --git a/block_int.h b/block_int.h
index 50e1a0b..9b70868 100644
--- a/block_int.h
+++ b/block_int.h
@@ -120,6 +120,8 @@  struct BlockDriver {
     /* Returns number of errors in image, -errno for internal errors */
     int (*bdrv_check)(BlockDriverState* bs);
 
+    void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
+
     /* Set if newly created images are not guaranteed to contain only zeros */
     int no_zero_init;