Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 20/12/2021 11:47, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2021 13:34, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
>>>> 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*.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>   block/mirror.c | 8 +++-----
>>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>> index 00089e519b..f22fa7da6e 100644
>>>> --- a/block/mirror.c
>>>> +++ b/block/mirror.c
>>>> @@ -653,7 +653,7 @@ 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 = job_has_failed(job);
>>>>       int ret = 0;
>>>>       if (s->prepared) {
>>>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error 
>>>> **errp)
>>>>       s->should_complete = true;
>>>>       /* If the job is paused, it will be re-entered when it is 
>>>> resumed */
>>>> -    if (!job->paused) {
>>>> -        job_enter(job);
>>>> -    }
>>>> +    job_enter_not_paused(job);
>>>>   }
>>>>   static void coroutine_fn mirror_pause(Job *job)
>>>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
>>>>        * from one of our own drain sections, to avoid a deadlock 
>>>> waiting for
>>>>        * ourselves.
>>>>        */
>>>> -    if (!s->common.job.paused && !job_is_cancelled(&job->job) && 
>>>> !s->in_drain) {
>>>> +    if (job_not_paused_nor_cancelled(&s->common.job) && 
>>>> !s->in_drain) {
>>>>           return true;
>>>>       }
>>>>
>>>
>>> Why to introduce a separate API function for every use case?
>>>
>>> Could we instead just use WITH_JOB_LOCK_GUARD() ?
>>>
>>
>> This implies making the struct job_mutex public. Is that ok for you?
>>
> 
> Yes, I think it's OK.
> 
> Alternatively, you can use job_lock() / job_unlock(), or even rewrite 
> WITH_JOB_LOCK_GUARD() macro using job_lock/job_unlock, to keep mutex 
> private.. But I don't think it really worth it now.
> 
> Note that struct Job is already public, so if we'll use per-job mutex in 
> future it still is not a problem. Only when we decide to make struct Job 
> private, we'll have to decide something about JOB_LOCK_GUARD(), and at 
> this point we'll just rewrite it to work through some helper function 
> instead of directly touching the mutex.
> 
> 

Ok I will do that. Just FYI the initial idea was that drivers like 
monitor would not need to know about job_mutex lock, that is why I made 
the helpers in mirror.c.

Thank you,
Emanuele