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.
> 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 <js...@redhat.com>
>
> - return -error;
> + return ret;
> }
>
> static int coroutine_fn
>
