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