[Qemu-devel] [PATCH 0/3] blkdebug: Fix config with multiple states

[Qemu-devel] [PATCH 0/3] blkdebug: Fix config with multiple states

[Kevin Wolf]

Turns out that using more than one state doesn't really work well. I'm trying
to reproduce a bug for which I need states, so now is the time to fix it.

Kevin Wolf (3):
  blkdebug: Fix set_state_opts definition
  blkdebug: Free QemuOpts after having read the config
  blkdebug: Initialize state as 1

 block/blkdebug.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH 1/3] blkdebug: Fix set_state_opts definition

[Kevin Wolf]

The list head was initialized to point to the wrong list, so all actions ended
up being handled as inject-error even if they were set-state in fact.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 98fed94..4ec8ca6 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -111,7 +111,7 @@ static QemuOptsList inject_error_opts = {
 
 static QemuOptsList set_state_opts = {
     .name = "set-state",
-    .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
+    .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head),
     .desc = {
         {
             .name = "event",
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/3] blkdebug: Free QemuOpts after having read the config

[Kevin Wolf]

Forgetting to free them means that the next instance inherits all rules and
gets its own rules only additionally.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 4ec8ca6..b084782 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -242,6 +242,11 @@ static int add_rule(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static int free_opts(QemuOpts *opts, void *opaque) {
+    qemu_opts_del(opts);
+    return 0;
+}
+
 static int read_config(BDRVBlkdebugState *s, const char *filename)
 {
     FILE *f;
@@ -267,6 +272,8 @@ static int read_config(BDRVBlkdebugState *s, const char *filename)
 
     ret = 0;
 fail:
+    qemu_opts_foreach(&inject_error_opts, free_opts, NULL, 0);
+    qemu_opts_foreach(&set_state_opts, free_opts, NULL, 0);
     fclose(f);
     return ret;
 }
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 2/3] blkdebug: Free QemuOpts after having read the config

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Forgetting to free them means that the next instance inherits all rules and
> gets its own rules only additionally.

I also found a use for freeing a complete QemuOptsList, here's my
solution.  The code that needs it isn't ready, yet.  If you'd like to
use it, I can push it to my repo.


diff --git a/qemu-option.c b/qemu-option.c
index 7f70d0f..30327d4 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -719,6 +719,15 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exist
     return opts;
 }
 
+void qemu_opts_reset(QemuOptsList *list)
+{
+    QemuOpts *opts, *next_opts;
+
+    QTAILQ_FOREACH_SAFE(opts, &list->head, next, next_opts) {
+        qemu_opts_del(opts);
+    }
+}
+
 int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value)
 {
diff --git a/qemu-option.h b/qemu-option.h
index 4823219..9e2406c 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -115,6 +115,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
 
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists);
+void qemu_opts_reset(QemuOptsList *list);
 int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value);
 const char *qemu_opts_id(QemuOpts *opts);

Re: [Qemu-devel] [PATCH 2/3] blkdebug: Free QemuOpts after having read the config

[Kevin Wolf]

Am 01.07.2010 08:47, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Forgetting to free them means that the next instance inherits all rules and
>> gets its own rules only additionally.
> 
> I also found a use for freeing a complete QemuOptsList, here's my
> solution.  The code that needs it isn't ready, yet.  If you'd like to
> use it, I can push it to my repo.

If you have a use for it, it's the better solution. I'll rebase on top
of this as soon as you have sent it as a proper patch.

Kevin

[Qemu-devel] [PATCH 3/3] blkdebug: Initialize state as 1

[Kevin Wolf]

state = 0 in rules means that the rule is valid for any state. Therefore it's
impossible to have a rule that works only in the initial state. This changes
the initial state from 0 to 1 to make this possible.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b084782..99340db 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -306,6 +306,9 @@ static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags)
     }
     filename = c + 1;
 
+    /* Set initial state */
+    s->vars.state = 1;
+
     /* Open the backing file */
     ret = bdrv_file_open(&bs->file, filename, flags);
     if (ret < 0) {
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 0/3] blkdebug: Fix config with multiple states

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Turns out that using more than one state doesn't really work well. I'm trying
> to reproduce a bug for which I need states, so now is the time to fix it.

I'm not familiar with blkdebug, but these look like obvious bug fixes.