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

[John Snow <jsnow@redhat.com>]

On 02/06/2015 04:15 PM, Max Reitz wrote:
> 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
>

augh, durr.

Because I didn't want the deletion of the rule to trigger a segfault, 
but I wasn't thinking about the interaction of both rules here. Duhhh.