Re: [Qemu-devel] [PATCH v2] blkdebug: fix "once" rule

[Max Reitz <mreitz@redhat.com>]

On 2015-02-06 at 13:23, John Snow wrote:
> Background:
>    The blkdebug scripts are currently engineered so that when a debug
> event occurs, a prefilter browses a master list of parsed rules for a
> certain event and adds them to an "active list" of rules to be used for
> the forthcoming action, provided the events and state numbers match.
>
>    Then, once the request is received, the last active rule is used to
> inject an error if certain parameters match.
>
>    This active list is cleared every time the prefilter injects a new
> rule for the first time during a debug event.
>
>    The "once" rule currently causes the error injection, if it is
> triggered, to only clear the active list. This is insufficient for
> preventing future injections of the same rule.
>
> Remedy:
>    This patch /deletes/ the rule from the list that the prefilter
> browses, so it is gone for good. In V2, we remove only the rule of
> interest from the active list instead of allowing the "once" rule to
> clear the entire list of active rules.
>
> Impact:
>    This affects iotests 026. Several ENOSPC tests that used "once" can
> be seen to have output that shows multiple failure messages. After
> this patch, the error messages tend to be smaller and less severe, but
> the injection can still be seen to be working. I have patched the
> expected output to expect the smaller error messages.
>
> V2:
>    - Remove only the offending "once" rule from the active list instead
>      of clearing the entire active list.

Could you move this version information under the three dashes so it 
won't be included in the commit message?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Here, that is.

>   block/blkdebug.c           |  9 +++++----
>   tests/qemu-iotests/026.out | 24 ++++--------------------
>   2 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 9ce35cd..185695b 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -473,14 +473,15 @@ static BlockAIOCB *inject_error(BlockDriverState *bs,
>       struct BlkdebugAIOCB *acb;
>       QEMUBH *bh;
>   
> -    if (rule->options.inject.once) {
> -        QSIMPLEQ_INIT(&s->active_rules);
> -    }
> -
>       if (rule->options.inject.immediately) {
>           return NULL;
>       }
>   
> +    if (rule->options.inject.once) {
> +        QSIMPLEQ_REMOVE(&s->active_rules, rule, BlkdebugRule, active_next);
> +        remove_rule(rule);
> +    }
> +
>       acb = qemu_aio_get(&blkdebug_aiocb_info, bs, cb, opaque);
>       acb->ret = -error;
>   

The new code itself looks good, but why did you move it after the 
immediately block? Because now, if "immediately" is set, "once" will be 
ignored:

$ ./qemu-img create -f qcow2 test.qcow2 64M; ./qemu-io -c 'aio_write 0 
64k' 
"json:{'driver':'qcow2','file':{'driver':'blkdebug','image':{'driver':'file','filename':'test.qcow2'},'inject-error':[{'event':'write_aio','once':true,'immediately':true}]}}" 

Formatting 'test.qcow2', fmt=qcow2 size=67108864 encryption=off 
cluster_size=65536 lazy_refcounts=off
aio_write failed: Input/output error
Failed to flush the L2 table cache: Input/output error
Failed to flush the refcount block cache: Input/output error

(I know, I know, I should have noticed in v1...)

Max