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

[John Snow]

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 <address@hidden>
> ---
>  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.

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

Reviewed-By: John Snow <address@hidden>

>  
> -    return -error;
> +    return ret;
>  }
>  
>  static int coroutine_fn
>