| Message ID | 1542006398-30037-2-git-send-email-marcolso@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] blkdebug: fix one shot rule processing | expand |
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 <marcolso@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. > 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 <jsnow@redhat.com> > > - return -error; > + return ret; > } > > static int coroutine_fn >
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 <marcolso@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 <jsnow@redhat.com> > >> >> - return -error; >> + return ret; >> } >> >> static int coroutine_fn >>
On 11/13/18 6:34 PM, Marc Olson wrote: > 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 <marcolso@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? > Yes, I just mean to say that "once" and "offset" now get defined for set_state tagged rules, and could theoretically be specified by the user (I think?) I don't think it will change anything. Just pointing it out in case anyone knows better than I do. >> >>> 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 <jsnow@redhat.com> >> >>> - return -error; >>> + return ret; >>> } >>> static int coroutine_fn >>>
On 12.11.18 08:06, Marc Olson 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 <marcolso@amazon.com> > --- > block/blkdebug.c | 59 +++++++++++++++++++++++++++++--------------------------- > 1 file changed, 31 insertions(+), 28 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
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; 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; } - return -error; + return ret; } static int coroutine_fn
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 <marcolso@amazon.com> --- block/blkdebug.c | 59 +++++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 28 deletions(-)