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

[Marc Olson]

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


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 <address@hidden>
>
> >   
> > -    return -error;
> > +    return ret;
> >   }
> >   
> >   static int coroutine_fn