Re: [PATCH v5 3/6] blkdebug: track all actions

[Max Reitz <mreitz@redhat.com>]

On 14.06.21 10:29, Emanuele Giuseppe Esposito wrote:
> Add a counter for each action that a rule can trigger.
> This is mainly used to keep track of how many coroutine_yield()
> we need to perform after processing all rules in the list.
>
> Co-developed-by: Paolo Bonzini<pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   block/blkdebug.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index e8fdf7b056..6bdeb2c7b3 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -74,6 +74,7 @@ enum {
>       ACTION_INJECT_ERROR,
>       ACTION_SET_STATE,
>       ACTION_SUSPEND,
> +    ACTION__MAX,
>   };
>   
>   typedef struct BlkdebugRule {
> @@ -791,22 +792,22 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
>       qemu_coroutine_yield();
>   }
>   
> -static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> -    bool injected)
> +static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> +                         int *action_count)

I would have liked a comment above this function explaining that 
`action_count` is not merely an int pointer, but actually an 
int[ACTION__MAX] pointer.

But it’s too late to complain about that now. O:)

>   {
>       BDRVBlkdebugState *s = bs->opaque;
>   
>       /* Only process rules for the current state */
>       if (rule->state && rule->state != s->state) {
> -        return injected;
> +        return;
>       }
>   
>       /* Take the action */
> +    action_count[rule->action]++;
>       switch (rule->action) {
>       case ACTION_INJECT_ERROR:
> -        if (!injected) {
> +        if (action_count[ACTION_INJECT_ERROR] == 1) {
>               QSIMPLEQ_INIT(&s->active_rules);

(I don’t quite understand this part – why do we clear the list of active 
rules here?  And why only if a new error is injected?  For example, if I 
have an inject-error rule that should only fire on state 1, and then the 
state changes to state 2, it stays active until a new error is injected, 
which doesn’t make sense to me.  But that has nothing to do with this 
series, of course.  I’m just wondering.)

Max

> -            injected = true;
>           }
>           QSIMPLEQ_INSERT_HEAD(&s->active_rules, rule, active_next);
>           break;
>
>    
>