Re: [PATCH v4 5/6] block/blkdebug: remove new_state field and instead use a local variable

[Vladimir Sementsov-Ogievskiy]

04.06.2021 13:07, Emanuele Giuseppe Esposito wrote:
> There seems to be no benefit in using a field. Replace it with a local
> variable.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/blkdebug.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index dffd869b32..d597753139 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -40,7 +40,6 @@
>   
>   typedef struct BDRVBlkdebugState {
>       int state;
> -    int new_state;
>       uint64_t align;
>       uint64_t max_transfer;
>       uint64_t opt_write_zero;
> @@ -792,7 +791,7 @@ static void suspend_request(BlockDriverState *bs, 
> BlkdebugRule *rule)
>   }
>   
>   static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> -                         int *action_count)
> +                         int *action_count, int *new_state)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
>   
> @@ -812,7 +811,8 @@ static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
>           break;
>   
>       case ACTION_SET_STATE:
> -        s->new_state = rule->options.set_state.new_state;
> +        assert(new_state != NULL);

you don't need additional assertion: crash on dereferencing NULL pointer is not 
less clear

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +        *new_state = rule->options.set_state.new_state;
>           break;
>   
>       case ACTION_SUSPEND:
> @@ -825,13 +825,14 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)
>   {
>       BDRVBlkdebugState *s = bs->opaque;
>       struct BlkdebugRule *rule, *next;
> +    int new_state;
>       int actions_count[ACTION__MAX] = { 0 };
>   
>       assert((int)event >= 0 && event < BLKDBG__MAX);
>   
> -    s->new_state = s->state;
> +    new_state = s->state;
>       QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> -        process_rule(bs, rule, actions_count);
> +        process_rule(bs, rule, actions_count, &new_state);
>       }
>   
>       while (actions_count[ACTION_SUSPEND] > 0) {
> @@ -839,7 +840,7 @@ static void blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)
>           actions_count[ACTION_SUSPEND]--;
>       }
>   
> -    s->state = s->new_state;
> +    s->state = new_state;
>   }
>   
>   static int blkdebug_debug_breakpoint(BlockDriverState *bs, const char *event,


--
Best regards,
Vladimir