[Qemu-devel] [RFC][PATCH 0/7] blkdebug

[Qemu-devel] [RFC][PATCH 0/7] blkdebug

[Kevin Wolf]

This patch series introduces a new block driver which acts as a protocol and
whose purpose it is to fail requests. To be more precise, I want it to fail in
configurable places, so that qemu-iotests can be extended with tests for the
error paths (for example for the case when something with metadata writes goes
wrong deep in qcow2).

It works like this (I think this is self-explanatory):

$ cat /tmp/blkdebug.cfg
[inject-error]
event = "l1_update"
errno = "5"
immediately = "on"
$ qemu-io blkdebug:/tmp/blkdebug.cfg:/tmp/empty.qcow2
qemu-io> read 0 4k
read 4096/4096 bytes at offset 0
4 KiB, 1 ops; 0.0000 sec (195.312 MiB/sec and 50000.0000 ops/sec)
qemu-io> write 0 4k
bdrv_debug_event: 4
blkdebug_debug_event: 4
write failed: Input/output error

Basically what I think is left to do is:
- Resolve TODOs and FIXMEs left in the code
- Require blkdebug to be explicitly enabled before compiling in event
  generation (not sure if it really matters)
- Add more events, they are only added for qcow2-clusters so far
- Address your comments (if any)

Kevin Wolf (7):
  qemu-config: qemu_read_config_file() reads the normal config file
  qemu-config: Make qemu_config_parse more generic
  blkdebug: Basic request passthrough
  blkdebug: Inject errors
  Make qemu-config available for tools
  blkdebug: Add events and rules
  qcow2: Trigger blkdebug events

 Makefile.objs         |    6 +-
 block.c               |   13 ++
 block.h               |   28 ++++
 block/blkdebug.c      |  423 +++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2-cluster.c |   15 ++
 block_int.h           |    2 +
 hw/qdev-properties.c  |   19 ++-
 hw/qdev.h             |    1 -
 qemu-config.c         |   45 +++---
 qemu-config.h         |    4 +-
 vl.c                  |   34 ++---
 11 files changed, 540 insertions(+), 50 deletions(-)
 create mode 100644 block/blkdebug.c

[Qemu-devel] [RFC][PATCH 1/7] qemu-config: qemu_read_config_file() reads the normal config file

[Kevin Wolf]

Introduce a new function qemu_read_config_file which reads the VM configuration
from a config file. Unlike qemu_config_parse it doesn't take a open file but a
filename and reduces code duplication as a side effect.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-config.c |   15 +++++++++++++++
 qemu-config.h |    2 ++
 vl.c          |   34 +++++++++++++---------------------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 246fae6..8166b5e 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -473,3 +473,18 @@ int qemu_config_parse(FILE *fp)
     }
     return 0;
 }
+
+int qemu_read_config_file(const char *filename)
+{
+    FILE *f = fopen(filename, "r");
+    if (f == NULL) {
+        return -errno;
+    }
+
+    if (qemu_config_parse(f) != 0) {
+        return -EINVAL;
+    }
+    fclose(f);
+
+    return 0;
+}
diff --git a/qemu-config.h b/qemu-config.h
index b335c42..bbe9d41 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -18,4 +18,6 @@ void qemu_add_globals(void);
 void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp);
 
+int qemu_read_config_file(const char *filename);
+
 #endif /* QEMU_CONFIG_H */
diff --git a/vl.c b/vl.c
index a3e43ad..078e3f9 100644
--- a/vl.c
+++ b/vl.c
@@ -4940,21 +4940,17 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (defconfig) {
-        FILE *fp;
-        fp = fopen(CONFIG_QEMU_CONFDIR "/qemu.conf", "r");
-        if (fp) {
-            if (qemu_config_parse(fp) != 0) {
-                exit(1);
-            }
-            fclose(fp);
+        int ret;
+
+        ret = qemu_read_config_file(CONFIG_QEMU_CONFDIR "/qemu.conf");
+        if (ret == -EINVAL) {
+            exit(1);
         }
 
-        fp = fopen(CONFIG_QEMU_CONFDIR "/target-" TARGET_ARCH ".conf", "r");
-        if (fp) {
-            if (qemu_config_parse(fp) != 0) {
-                exit(1);
-            }
-            fclose(fp);
+        ret = qemu_read_config_file(CONFIG_QEMU_CONFDIR
+            "/target-" TARGET_ARCH ".conf");
+        if (ret == -EINVAL) {
+            exit(1);
         }
     }
 #if defined(cpudef_setup)
@@ -5635,16 +5631,12 @@ int main(int argc, char **argv, char **envp)
 #endif
             case QEMU_OPTION_readconfig:
                 {
-                    FILE *fp;
-                    fp = fopen(optarg, "r");
-                    if (fp == NULL) {
-                        fprintf(stderr, "open %s: %s\n", optarg, strerror(errno));
+                    int ret = qemu_read_config_file(optarg);
+                    if (ret < 0) {
+                        fprintf(stderr, "read config %s: %s\n", optarg,
+                            strerror(-ret));
                         exit(1);
                     }
-                    if (qemu_config_parse(fp) != 0) {
-                        exit(1);
-                    }
-                    fclose(fp);
                     break;
                 }
             case QEMU_OPTION_writeconfig:
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 1/7] qemu-config: qemu_read_config_file() reads the normal config file

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:29PM +0100, Kevin Wolf wrote:
> Introduce a new function qemu_read_config_file which reads the VM configuration
> from a config file. Unlike qemu_config_parse it doesn't take a open file but a
> filename and reduces code duplication as a side effect.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [RFC][PATCH 2/7] qemu-config: Make qemu_config_parse more generic

[Kevin Wolf]

qemu_config_parse gets the option groups as a parameter now instead of
hardcoding the VM configuration groups. This way it can be used for other
configurations, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-config.c |   15 ++++++++-------
 qemu-config.h |    2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 8166b5e..e121f2a 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -290,7 +290,7 @@ QemuOptsList qemu_cpudef_opts = {
     },
 };
 
-static QemuOptsList *lists[] = {
+static QemuOptsList *vm_config_groups[] = {
     &qemu_drive_opts,
     &qemu_chardev_opts,
     &qemu_device_opts,
@@ -303,7 +303,7 @@ static QemuOptsList *lists[] = {
     NULL,
 };
 
-static QemuOptsList *find_list(const char *group)
+static QemuOptsList *find_list(QemuOptsList **lists, const char *group)
 {
     int i;
 
@@ -330,7 +330,7 @@ int qemu_set_option(const char *str)
         return -1;
     }
 
-    list = find_list(group);
+    list = find_list(vm_config_groups, group);
     if (list == NULL) {
         return -1;
     }
@@ -415,6 +415,7 @@ static int config_write_opts(QemuOpts *opts, void *opaque)
 void qemu_config_write(FILE *fp)
 {
     struct ConfigWriteData data = { .fp = fp };
+    QemuOptsList **lists = vm_config_groups;
     int i;
 
     fprintf(fp, "# qemu config file\n\n");
@@ -424,7 +425,7 @@ void qemu_config_write(FILE *fp)
     }
 }
 
-int qemu_config_parse(FILE *fp)
+int qemu_config_parse(FILE *fp, QemuOptsList **lists)
 {
     char line[1024], group[64], id[64], arg[64], value[1024];
     QemuOptsList *list = NULL;
@@ -441,7 +442,7 @@ int qemu_config_parse(FILE *fp)
         }
         if (sscanf(line, "[%63s \"%63[^\"]\"]", group, id) == 2) {
             /* group with id */
-            list = find_list(group);
+            list = find_list(lists, group);
             if (list == NULL)
                 return -1;
             opts = qemu_opts_create(list, id, 1);
@@ -449,7 +450,7 @@ int qemu_config_parse(FILE *fp)
         }
         if (sscanf(line, "[%63[^]]]", group) == 1) {
             /* group without id */
-            list = find_list(group);
+            list = find_list(lists, group);
             if (list == NULL)
                 return -1;
             opts = qemu_opts_create(list, NULL, 0);
@@ -481,7 +482,7 @@ int qemu_read_config_file(const char *filename)
         return -errno;
     }
 
-    if (qemu_config_parse(f) != 0) {
+    if (qemu_config_parse(f, vm_config_groups) != 0) {
         return -EINVAL;
     }
     fclose(f);
diff --git a/qemu-config.h b/qemu-config.h
index bbe9d41..086aa2a 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -16,7 +16,7 @@ int qemu_global_option(const char *str);
 void qemu_add_globals(void);
 
 void qemu_config_write(FILE *fp);
-int qemu_config_parse(FILE *fp);
+int qemu_config_parse(FILE *fp, QemuOptsList **lists);
 
 int qemu_read_config_file(const char *filename);
 
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 2/7] qemu-config: Make qemu_config_parse more generic

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:30PM +0100, Kevin Wolf wrote:
> qemu_config_parse gets the option groups as a parameter now instead of
> hardcoding the VM configuration groups. This way it can be used for other
> configurations, too.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [RFC][PATCH 3/7] blkdebug: Basic request passthrough

[Kevin Wolf]

This isn't doing anything interesting. It creates the blkdebug block driver as
a protocol which just passes everything through to raw.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 Makefile.objs    |    2 +-
 block/blkdebug.c |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletions(-)
 create mode 100644 block/blkdebug.c

diff --git a/Makefile.objs b/Makefile.objs
index e791dd5..ece02d5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,7 +14,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
 block-nested-y += cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
 block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
-block-nested-y += parallels.o nbd.o
+block-nested-y += parallels.o nbd.o blkdebug.o
 block-nested-$(CONFIG_WIN32) += raw-win32.o
 block-nested-$(CONFIG_POSIX) += raw-posix.o
 block-nested-$(CONFIG_CURL) += curl.o
diff --git a/block/blkdebug.c b/block/blkdebug.c
new file mode 100644
index 0000000..2c7e0dd
--- /dev/null
+++ b/block/blkdebug.c
@@ -0,0 +1,104 @@
+/*
+ * Block protocol for I/O error injection
+ *
+ * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu-common.h"
+#include "block_int.h"
+#include "module.h"
+
+typedef struct BDRVBlkdebugState {
+    BlockDriverState *hd;
+} BDRVBlkdebugState;
+
+static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+
+    if (strncmp(filename, "blkdebug:", strlen("blkdebug:"))) {
+        return -EINVAL;
+    }
+    filename += strlen("blkdebug:");
+
+    return bdrv_file_open(&s->hd, filename, flags);
+}
+
+static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
+    int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlockDriverAIOCB *acb =
+        bdrv_aio_readv(s->hd, sector_num, qiov, nb_sectors, cb, opaque);
+    return acb;
+}
+
+static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
+    int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlockDriverAIOCB *acb =
+        bdrv_aio_writev(s->hd, sector_num, qiov, nb_sectors, cb, opaque);
+    return acb;
+}
+
+static void blkdebug_close(BlockDriverState *bs)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    bdrv_delete(s->hd);
+}
+
+static void blkdebug_flush(BlockDriverState *bs)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    bdrv_flush(s->hd);
+}
+
+static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    return bdrv_aio_flush(s->hd, cb, opaque);
+}
+
+static BlockDriver bdrv_blkdebug = {
+    .format_name        = "blkdebug",
+    .protocol_name      = "blkdebug",
+
+    .instance_size      = sizeof(BDRVBlkdebugState),
+
+    .bdrv_open          = blkdebug_open,
+    .bdrv_close         = blkdebug_close,
+    .bdrv_flush         = blkdebug_flush,
+
+    .bdrv_aio_readv     = blkdebug_aio_readv,
+    .bdrv_aio_writev    = blkdebug_aio_writev,
+    .bdrv_aio_flush     = blkdebug_aio_flush,
+};
+
+static void bdrv_blkdebug_init(void)
+{
+    bdrv_register(&bdrv_blkdebug);
+}
+
+block_init(bdrv_blkdebug_init);
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 3/7] blkdebug: Basic request passthrough

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:31PM +0100, Kevin Wolf wrote:
> This isn't doing anything interesting. It creates the blkdebug block driver as
> a protocol which just passes everything through to raw.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  Makefile.objs    |    2 +-
>  block/blkdebug.c |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++

Not a big fan of the blkdebug: prefix, but that's how qemu works
elsewhere, too.  Also it's not passing through any of the ioctl
magic, but that's probably not needed for now.


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [RFC][PATCH 4/7] blkdebug: Inject errors

[Kevin Wolf]

Add a mechanism to inject errors instead of passing requests on. With no
further patches applied, you can use it by setting inject_errno in gdb.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2c7e0dd..f8ccd3c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -26,10 +26,26 @@
 #include "block_int.h"
 #include "module.h"
 
+#include <stdbool.h>
+
 typedef struct BDRVBlkdebugState {
     BlockDriverState *hd;
+
+    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;
 } BDRVBlkdebugState;
 
+struct callback_data {
+    QEMUBH *bh;
+    BlockDriverCompletionFunc *cb;
+    int ret;
+    void *opaque;
+};
+
 static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -42,11 +58,53 @@ static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
     return bdrv_file_open(&s->hd, filename, flags);
 }
 
+static void error_callback_bh(void *opaque)
+{
+    struct callback_data *d = opaque;
+    qemu_bh_delete(d->bh);
+    d->cb(d->opaque, d->ret);
+    qemu_free(d);
+}
+
+static BlockDriverAIOCB *inject_error(BlockDriverState *bs,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    int error = s->inject_errno;
+    struct callback_data *d;
+    QEMUBH *bh;
+
+    if (s->inject_once) {
+        s->inject_errno = 0;
+    }
+
+    if (s->inject_immediately) {
+        return NULL;
+    }
+
+    d = qemu_mallocz(sizeof(*d));
+    d->ret = error;
+    d->opaque = opaque;
+    d->cb = cb;
+
+    bh = qemu_bh_new(error_callback_bh, d);
+    d->bh = bh;
+    qemu_bh_schedule(bh);
+
+    // TODO Use a real AIOCB
+    return (BlockDriverAIOCB*) 42;
+}
+
 static BlockDriverAIOCB *blkdebug_aio_readv(BlockDriverState *bs,
     int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
+
+    if (s->inject_errno) {
+        return inject_error(bs, cb, opaque);
+    }
+
     BlockDriverAIOCB *acb =
         bdrv_aio_readv(s->hd, sector_num, qiov, nb_sectors, cb, opaque);
     return acb;
@@ -57,6 +115,11 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
     BlockDriverCompletionFunc *cb, void *opaque)
 {
     BDRVBlkdebugState *s = bs->opaque;
+
+    if (s->inject_errno) {
+        return inject_error(bs, cb, opaque);
+    }
+
     BlockDriverAIOCB *acb =
         bdrv_aio_writev(s->hd, sector_num, qiov, nb_sectors, cb, opaque);
     return acb;
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 4/7] blkdebug: Inject errors

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:32PM +0100, Kevin Wolf wrote:
> Add a mechanism to inject errors instead of passing requests on. With no
> further patches applied, you can use it by setting inject_errno in gdb.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

The magic iocb is quite a hack, but it should work good enough for now.


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [RFC][PATCH 5/7] Make qemu-config available for tools

[Kevin Wolf]

To be able to use config files for blkdebug, we need to make these functions
available in the tools. This involves moving two functions that can only be
built in the context of the emulator.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 Makefile.objs        |    4 ++--
 hw/qdev-properties.c |   19 ++++++++++++++++++-
 hw/qdev.h            |    1 -
 qemu-config.c        |   17 -----------------
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index ece02d5..930408b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -8,7 +8,7 @@ qobject-obj-y += qerror.o
 # block-obj-y is code used by both qemu system emulation and qemu-img
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
-block-obj-y += nbd.o block.o aio.o aes.o osdep.o
+block-obj-y += nbd.o block.o aio.o aes.o osdep.o qemu-config.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
@@ -76,7 +76,7 @@ common-obj-y += buffered_file.o migration.o migration-tcp.o qemu-sockets.o
 common-obj-y += qemu-char.o savevm.o #aio.o
 common-obj-y += msmouse.o ps2.o
 common-obj-y += qdev.o qdev-properties.o
-common-obj-y += qemu-config.o block-migration.o
+common-obj-y += block-migration.o
 
 common-obj-$(CONFIG_BRLAPI) += baum.o
 common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 24671af..35d284e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -666,7 +666,7 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 
 static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
 
-void qdev_prop_register_global(GlobalProperty *prop)
+static void qdev_prop_register_global(GlobalProperty *prop)
 {
     QTAILQ_INSERT_TAIL(&global_props, prop, next);
 }
@@ -694,3 +694,20 @@ void qdev_prop_set_globals(DeviceState *dev)
         }
     }
 }
+
+static int qdev_add_one_global(QemuOpts *opts, void *opaque)
+{
+    GlobalProperty *g;
+
+    g = qemu_mallocz(sizeof(*g));
+    g->driver   = qemu_opt_get(opts, "driver");
+    g->property = qemu_opt_get(opts, "property");
+    g->value    = qemu_opt_get(opts, "value");
+    qdev_prop_register_global(g);
+    return 0;
+}
+
+void qemu_add_globals(void)
+{
+    qemu_opts_foreach(&qemu_global_opts, qdev_add_one_global, NULL, 0);
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index adfcf79..30f31a1 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -273,7 +273,6 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
-void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
 
diff --git a/qemu-config.c b/qemu-config.c
index e121f2a..9070729 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -367,23 +367,6 @@ int qemu_global_option(const char *str)
     return 0;
 }
 
-static int qemu_add_one_global(QemuOpts *opts, void *opaque)
-{
-    GlobalProperty *g;
-
-    g = qemu_mallocz(sizeof(*g));
-    g->driver   = qemu_opt_get(opts, "driver");
-    g->property = qemu_opt_get(opts, "property");
-    g->value    = qemu_opt_get(opts, "value");
-    qdev_prop_register_global(g);
-    return 0;
-}
-
-void qemu_add_globals(void)
-{
-    qemu_opts_foreach(&qemu_global_opts, qemu_add_one_global, NULL, 0);
-}
-
 struct ConfigWriteData {
     QemuOptsList *list;
     FILE *fp;
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 5/7] Make qemu-config available for tools

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:33PM +0100, Kevin Wolf wrote:
> available in the tools. This involves moving two functions that can only be
> built in the context of the emulator.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [RFC][PATCH 6/7] blkdebug: Add events and rules

[Kevin Wolf]

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(-)

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;
 
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 6/7] blkdebug: Add events and rules

[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?

> +#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?

Re: [Qemu-devel] [RFC][PATCH 6/7] blkdebug: Add events and rules

[Kevin Wolf]

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

[Qemu-devel] [RFC][PATCH 7/7] qcow2: Trigger blkdebug events

[Kevin Wolf]

This adds blkdebug events to qcow2 to allow injecting I/O errors in specific
places.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.h               |   19 +++++++++++++++++++
 block/blkdebug.c      |   17 +++++++++++++++++
 block/qcow2-cluster.c |   15 +++++++++++++++
 3 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/block.h b/block.h
index 2fd8361..c0f17aa 100644
--- a/block.h
+++ b/block.h
@@ -210,6 +210,25 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 
 
 typedef enum {
+    BLKDBG_L1_UPDATE,
+
+    BLKDBG_L1_GROW_ALLOC_TABLE,
+    BLKDBG_L1_GROW_WRITE_TABLE,
+    BLKDBG_L1_GROW_ACTIVATE_TABLE,
+
+    BLKDBG_L2_LOAD,
+    BLKDBG_L2_UPDATE,
+    BLKDBG_L2_UPDATE_COMPRESSED,
+    BLKDBG_L2_ALLOC_COW_READ,
+    BLKDBG_L2_ALLOC_WRITE,
+
+    BLKDBG_READ,
+    BLKDBG_READ_BACKING,
+    BLKDBG_READ_COMPRESSED,
+
+    BLKDBG_COW_READ,
+    BLKDBG_COW_WRITE,
+
     BLKDBG_EVENT_MAX,
 } BlkDebugEvent;
 
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 22b1768..2398975 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -126,6 +126,23 @@ static QemuOptsList *config_groups[] = {
 };
 
 static const char *event_names[BLKDBG_EVENT_MAX] = {
+    [BLKDBG_L1_UPDATE]              = "l1_update",
+    [BLKDBG_L1_GROW_ALLOC_TABLE]    = "l1_grow.alloc_table",
+    [BLKDBG_L1_GROW_WRITE_TABLE]    = "l1_grow.write_table",
+    [BLKDBG_L1_GROW_ACTIVATE_TABLE] = "l1_grow.activate_table",
+
+    [BLKDBG_L2_LOAD]                = "l2_load",
+    [BLKDBG_L2_UPDATE]              = "l2_update",
+    [BLKDBG_L2_UPDATE_COMPRESSED]   = "l2_update_compressed",
+    [BLKDBG_L2_ALLOC_COW_READ]      = "l2_alloc.cow_read",
+    [BLKDBG_L2_ALLOC_WRITE]         = "l2_alloc.write",
+
+    [BLKDBG_READ]                   = "read",
+    [BLKDBG_READ_BACKING]           = "read_backing",
+    [BLKDBG_READ_COMPRESSED]        = "read_compressed",
+
+    [BLKDBG_COW_READ]               = "cow_read",
+    [BLKDBG_COW_WRITE]              = "cow_write",
 };
 
 static int get_event_by_name(const char *name, BlkDebugEvent *event)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b13b693..9b3d686 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -54,12 +54,14 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
     memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
 
     /* write new table (align to cluster) */
+    BLKDBG_EVENT(s->hd, BLKDBG_L1_GROW_ALLOC_TABLE);
     new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
     if (new_l1_table_offset < 0) {
         qemu_free(new_l1_table);
         return new_l1_table_offset;
     }
 
+    BLKDBG_EVENT(s->hd, BLKDBG_L1_GROW_WRITE_TABLE);
     for(i = 0; i < s->l1_size; i++)
         new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
     ret = bdrv_pwrite(s->hd, new_l1_table_offset, new_l1_table, new_l1_size2);
@@ -69,6 +71,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
         new_l1_table[i] = be64_to_cpu(new_l1_table[i]);
 
     /* set new table */
+    BLKDBG_EVENT(s->hd, BLKDBG_L1_GROW_ACTIVATE_TABLE);
     cpu_to_be32w((uint32_t*)data, new_l1_size);
     cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
     ret = bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,sizeof(data));
@@ -170,6 +173,8 @@ static uint64_t *l2_load(BlockDriverState *bs, uint64_t l2_offset)
 
     min_index = l2_cache_new_entry(bs);
     l2_table = s->l2_cache + (min_index << s->l2_bits);
+
+    BLKDBG_EVENT(s->hd, BLKDBG_L2_LOAD);
     if (bdrv_pread(s->hd, l2_offset, l2_table, s->l2_size * sizeof(uint64_t)) !=
         s->l2_size * sizeof(uint64_t))
         return NULL;
@@ -195,6 +200,7 @@ static int write_l1_entry(BDRVQcowState *s, int l1_index)
         buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
     }
 
+    BLKDBG_EVENT(s->hd, BLKDBG_L1_UPDATE);
     if (bdrv_pwrite(s->hd, s->l1_table_offset + 8 * l1_start_index,
         buf, sizeof(buf)) != sizeof(buf))
     {
@@ -248,12 +254,14 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
         memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
     } else {
         /* if there was an old l2 table, read it from the disk */
+        BLKDBG_EVENT(s->hd, BLKDBG_L2_ALLOC_COW_READ);
         if (bdrv_pread(s->hd, old_l2_offset,
                        l2_table, s->l2_size * sizeof(uint64_t)) !=
             s->l2_size * sizeof(uint64_t))
             return NULL;
     }
     /* write the l2 table to the file */
+    BLKDBG_EVENT(s->hd, BLKDBG_L2_ALLOC_WRITE);
     if (bdrv_pwrite(s->hd, l2_offset,
                     l2_table, s->l2_size * sizeof(uint64_t)) !=
         s->l2_size * sizeof(uint64_t))
@@ -335,6 +343,7 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
                 /* read from the base image */
                 n1 = qcow2_backing_read1(bs->backing_hd, sector_num, buf, n);
                 if (n1 > 0) {
+                    BLKDBG_EVENT(s->hd, BLKDBG_READ_BACKING);
                     ret = bdrv_read(bs->backing_hd, sector_num, buf, n1);
                     if (ret < 0)
                         return -1;
@@ -347,6 +356,7 @@ static int qcow_read(BlockDriverState *bs, int64_t sector_num,
                 return -1;
             memcpy(buf, s->cluster_cache + index_in_cluster * 512, 512 * n);
         } else {
+            BLKDBG_EVENT(s->hd, BLKDBG_READ);
             ret = bdrv_pread(s->hd, cluster_offset + index_in_cluster * 512, buf, n * 512);
             if (ret != n * 512)
                 return -1;
@@ -371,6 +381,7 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
     n = n_end - n_start;
     if (n <= 0)
         return 0;
+    BLKDBG_EVENT(s->hd, BLKDBG_COW_READ);
     ret = qcow_read(bs, start_sect + n_start, s->cluster_data, n);
     if (ret < 0)
         return ret;
@@ -380,6 +391,7 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
                         s->cluster_data, n, 1,
                         &s->aes_encrypt_key);
     }
+    BLKDBG_EVENT(s->hd, BLKDBG_COW_WRITE);
     ret = bdrv_write(s->hd, (cluster_offset >> 9) + n_start,
                      s->cluster_data, n);
     if (ret < 0)
@@ -592,6 +604,7 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
 
     /* compressed clusters never have the copied flag */
 
+    BLKDBG_EVENT(s->hd, BLKDBG_L2_UPDATE_COMPRESSED);
     l2_table[l2_index] = cpu_to_be64(cluster_offset);
     if (bdrv_pwrite(s->hd,
                     l2_offset + l2_index * sizeof(uint64_t),
@@ -615,6 +628,7 @@ static int write_l2_entries(BDRVQcowState *s, uint64_t *l2_table,
     int end_offset = (8 * (l2_index + num) + 511) & ~511;
     size_t len = end_offset - start_offset;
 
+    BLKDBG_EVENT(s->hd, BLKDBG_L2_UPDATE);
     if (bdrv_pwrite(s->hd, l2_offset + start_offset, &l2_table[l2_start_index],
         len) != len)
     {
@@ -865,6 +879,7 @@ int qcow2_decompress_cluster(BDRVQcowState *s, uint64_t cluster_offset)
         nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
         sector_offset = coffset & 511;
         csize = nb_csectors * 512 - sector_offset;
+        BLKDBG_EVENT(s->hd, BLKDBG_READ_COMPRESSED);
         ret = bdrv_read(s->hd, coffset >> 9, s->cluster_data, nb_csectors);
         if (ret < 0) {
             return -1;
-- 
1.6.6.1

Re: [Qemu-devel] [RFC][PATCH 7/7] qcow2: Trigger blkdebug events

[Christoph Hellwig]

On Mon, Mar 15, 2010 at 06:08:35PM +0100, Kevin Wolf wrote:
> This adds blkdebug events to qcow2 to allow injecting I/O errors in specific
> places.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [Qemu-devel] [RFC][PATCH 0/7] blkdebug

[Blue Swirl]

On 3/15/10, Kevin Wolf <kwolf@redhat.com> wrote:
> This patch series introduces a new block driver which acts as a protocol and
>  whose purpose it is to fail requests. To be more precise, I want it to fail in
>  configurable places, so that qemu-iotests can be extended with tests for the
>  error paths (for example for the case when something with metadata writes goes
>  wrong deep in qcow2).

Nice. Do you think this could be extended (later) to inject errors
also to guest from QEMU? Then it would be nice to be able to inject
errors when reading/writing to a specific guest block range and for
other formats (raw etc.) too.

Re: [Qemu-devel] [RFC][PATCH 0/7] blkdebug

[Kevin Wolf]

Am 15.03.2010 18:40, schrieb Blue Swirl:
> On 3/15/10, Kevin Wolf <kwolf@redhat.com> wrote:
>> This patch series introduces a new block driver which acts as a protocol and
>>  whose purpose it is to fail requests. To be more precise, I want it to fail in
>>  configurable places, so that qemu-iotests can be extended with tests for the
>>  error paths (for example for the case when something with metadata writes goes
>>  wrong deep in qcow2).
> 
> Nice. Do you think this could be extended (later) to inject errors
> also to guest from QEMU? 

Not sure what you mean by "from QEMU"? To allow injecting errors from
the monitor instead of based on rules? Sounds certainly doable.

Or do you just mean to make it work in qemu as opposed to qemu-io? This
really doesn't make a difference to the block layer. If it works in one,
it works in the other one, too.

> Then it would be nice to be able to inject
> errors when reading/writing to a specific guest block range and for
> other formats (raw etc.) too.

Should work with all formats, it's implemented as a protocol (but it's
untested with everything but qcow2). So you basically stick it between
the format driver and the image file. The only thing that could be
needed for other formats is some additional events.

For the block range thing, this would need to implement some more
conditions instead of just checking the state, but in general this is
exactly the kind of things that I'd like to see supported eventually.

So I completely agree that this is the right direction for future
improvements, however I can't tell yet if (or when) I'll get to
implement it myself. My focus is really the qcow2 error paths for now.

Kevin

Re: [Qemu-devel] [RFC][PATCH 0/7] blkdebug

[Blue Swirl]

On 3/15/10, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 15.03.2010 18:40, schrieb Blue Swirl:
>
> > On 3/15/10, Kevin Wolf <kwolf@redhat.com> wrote:
>  >> This patch series introduces a new block driver which acts as a protocol and
>  >>  whose purpose it is to fail requests. To be more precise, I want it to fail in
>  >>  configurable places, so that qemu-iotests can be extended with tests for the
>  >>  error paths (for example for the case when something with metadata writes goes
>  >>  wrong deep in qcow2).
>  >
>  > Nice. Do you think this could be extended (later) to inject errors
>  > also to guest from QEMU?
>
>
> Not sure what you mean by "from QEMU"? To allow injecting errors from
>  the monitor instead of based on rules? Sounds certainly doable.

I was thinking rule file, but monitor would be more flexible. Even
better: allow switching rule files from monitor :-).

>  Or do you just mean to make it work in qemu as opposed to qemu-io? This
>  really doesn't make a difference to the block layer. If it works in one,
>  it works in the other one, too.
>
>
>  > Then it would be nice to be able to inject
>  > errors when reading/writing to a specific guest block range and for
>  > other formats (raw etc.) too.
>
>
> Should work with all formats, it's implemented as a protocol (but it's
>  untested with everything but qcow2). So you basically stick it between
>  the format driver and the image file. The only thing that could be
>  needed for other formats is some additional events.
>
>  For the block range thing, this would need to implement some more
>  conditions instead of just checking the state, but in general this is
>  exactly the kind of things that I'd like to see supported eventually.
>
>  So I completely agree that this is the right direction for future
>  improvements, however I can't tell yet if (or when) I'll get to
>  implement it myself. My focus is really the qcow2 error paths for now.

Ok, it's enough to know at this stage that the architecture is
flexible for this.