Re: [Qemu-devel] [PATCH v3 2/3] blkdebug: Extend rule check for additional types

[Marc Olson via Qemu-devel]

On 11/13/18 3:22 PM, John Snow wrote:

On 11/12/18 2:06 AM, Marc Olson via Qemu-devel wrote:

Break out the more common parts of the BlkdebugRule struct, and make
rule_check() more explicit about operating only on error injection types
so that additional rule types can be added in the future.

Signed-off-by: Marc Olson <marco...@amazon.com>
---
  block/blkdebug.c | 59 +++++++++++++++++++++++++++++---------------------------
  1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 327049b..7739849 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -73,13 +73,13 @@ typedef struct BlkdebugRule {
      BlkdebugEvent event;
      int action;
      int state;
+    int once;
+    int64_t offset;
      union {
          struct {
              int error;
              int immediately;
-            int once;
-            int64_t offset;
-        } inject;
+        } inject_error;

...pulling out "once" and "offset" from inject_error (renamed inject) to
shared properties. Fine, though this looks like it could use more love.
Not your doing.

This adds new dead fields for set_state and suspend which will now work,
but hopefully not do anything.

I think set_state was already there?

          struct {
              int new_state;
          } set_state;
@@ -182,16 +182,16 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
          .state  = qemu_opt_get_number(opts, "state", 0),
      };

+ rule->once = qemu_opt_get_bool(opts, "once", 0);

+    sector = qemu_opt_get_number(opts, "sector", -1);
+    rule->offset = sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
+
      /* 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 =
+        rule->options.inject_error.error = qemu_opt_get_number(opts, "errno", 
EIO);
+        rule->options.inject_error.immediately =
              qemu_opt_get_bool(opts, "immediately", 0);
-        sector = qemu_opt_get_number(opts, "sector", -1);
-        rule->options.inject.offset =
-            sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
          break;

case ACTION_SET_STATE:

@@ -474,38 +474,41 @@ static int rule_check(BlockDriverState *bs, uint64_t 
offset, uint64_t bytes)
  {
      BDRVBlkdebugState *s = bs->opaque;
      BlkdebugRule *rule = NULL;
+    BlkdebugRule *error_rule = NULL;
      int error;
      bool immediately;
+    int ret = 0;

QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {

-        uint64_t inject_offset = rule->options.inject.offset;
-
-        if (inject_offset == -1 ||
-            (bytes && inject_offset >= offset &&
-             inject_offset < offset + bytes))
+        if (rule->offset == -1 ||
+            (bytes && rule->offset >= offset &&
+             rule->offset < offset + bytes))
          {
-            break;
+            if (rule->action == ACTION_INJECT_ERROR) {
+                error_rule = rule;
+                break;
+            }
          }
      }

- if (!rule) {

-        return 0;
-    }
+    if (error_rule) {
+        immediately = error_rule->options.inject_error.immediately;
+        error = error_rule->options.inject_error.error;

- immediately = rule->options.inject.immediately;

-    error = rule->options.inject.error;
+        if (error_rule->once) {
+            QSIMPLEQ_REMOVE(&s->active_rules, error_rule, BlkdebugRule, 
active_next);
+            remove_rule(error_rule);
+        }

- if (rule->options.inject.once) {

-        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
-        remove_rule(rule);
-    }
+        if (error && !immediately) {
+            aio_co_schedule(qemu_get_current_aio_context(), 
qemu_coroutine_self());
+            qemu_coroutine_yield();
+        }

- if (error && !immediately) {

-        aio_co_schedule(qemu_get_current_aio_context(), qemu_coroutine_self());
-        qemu_coroutine_yield();
+        ret = -error;
      }

Bit messy as a diff, but it seems to check out. As a bonus we now
actually check the tag of the rules we're iterating through, so that
seems like an improvement.

Unfortunately git made a bit of a mess out of the diff.

Reviewed-By: John Snow <js...@redhat.com>

- return -error;

+    return ret;
  }

static int coroutine_fn