Am 04/08/2022 um 18:35 schrieb Kevin Wolf:
> Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben:
>> Once job lock is used and aiocontext is removed, mirror has
>> to perform job operations under the same critical section,
>> using the helpers prepared in previous commit.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
>> Reviewed-by: Stefan Hajnoczi <[email protected]>
>> Signed-off-by: Emanuele Giuseppe Esposito <[email protected]>
>
> Can you explain in the commit message what the TOC/TOU case is that this
> patch is addressing? It's not obvious to me why you picked exactly these
> places to add locking.
Well after thinking about it, mentioning TOC/TOU doesn't really make
sense here. I'll remove the "to avoid TOC/TOU" in the title.
>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index d8ecb9efa2..b38676e19d 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -654,9 +654,13 @@ static int mirror_exit_common(Job *job)
>> BlockDriverState *target_bs;
>> BlockDriverState *mirror_top_bs;
>> Error *local_err = NULL;
>> - bool abort = job->ret < 0;
>> + bool abort;
>> int ret = 0;
>>
>> + WITH_JOB_LOCK_GUARD() {
>> + abort = job->ret < 0;
>> + }
>
> This is the most mysterious hunk to me. The only thing that should
> modify job->ret is the caller of this function anyway, but let's assume
> for a moment that another thread could write to it.
>
> Then why is it only important that we hold the lock when we're reading
> the value, but not any more when we are actually using it? And what is
> the TOC/TOU that this fixes?
No TOC/TOU, no sense for this fix too. I'll remove this hunk too.
Emanuele
>
> Kevin
>
