Re: [PATCH v4 6/6] blkdebug: protect rules and suspended_reqs with a lock

[Paolo Bonzini]

On 04/06/21 12:07, Emanuele Giuseppe Esposito wrote:
> +    WITH_QEMU_LOCK_GUARD(&s->lock) {
> +        new_state = s->state;
> +        QLIST_FOREACH_SAFE(rule, &s->rules[event], next, next) {
> +            process_rule(bs, rule, actions_count, &new_state);
> +        }
> +        s->state = new_state;
>       }
>   
>       while (actions_count[ACTION_SUSPEND] > 0) {
>           qemu_coroutine_yield();
>           actions_count[ACTION_SUSPEND]--;
>       }
> -
> -    s->state = new_state;

This changes the semantics by moving the state change *before* the yield 
instead of after:

- before the series, the new state was assigned after all yields (and 
could be overwritten by other coroutines during the yield).  Until patch 
4, the situation is more or less the same even though the ordering 
changed in the processing of actions (suspend actions are processed last).

- with patch 5 new_state became a local variable, so it couldn't be 
overwritten by the yields

- now it is a local variable and is assigned before the yields.  The 
yields can write s->state just like before.

So it's a bit messy.  Moving s->state = new_state before the yields 
makes sense, but I'd do that in patch 5 to avoid the temporary change in 
semantics.

Paolo