Re: [RFC PATCH v2 13/14] jobs: add job lock in find_* functions

[Vladimir Sementsov-Ogievskiy]

18.12.2021 15:11, Vladimir Sementsov-Ogievskiy wrote:
> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote:
> > Both blockdev.c and job-qmp.c have TOC/TOU conditions, because
> > they first search for the job and then perform an action on it.
> > Therefore, we need to do the search + action under the same
> > job mutex critical section.
> >
> > Note: at this stage, job_{lock/unlock} and job lock guard macros
> > are *nop*.
> >
> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > ---
> >   blockdev.c | 9 +++++++++
> >   job-qmp.c  | 8 ++++++++
> >   2 files changed, 17 insertions(+)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index c5a835d9ed..0bd79757fc 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3327,12 +3327,14 @@ static BlockJob *find_block_job(const char *id, 
> > AioContext **aio_context,
> >       assert(id != NULL);
> >       *aio_context = NULL;
> > +    job_lock();
>
> JOB_LOCK_GUARD() will look better in this case
>
> >       job = block_job_get(id);
> >       if (!job) {
> >           error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> >                     "Block job '%s' not found", id);
> > +        job_unlock();
> >           return NULL;
> >       }
> > @@ -3353,6 +3355,7 @@ void qmp_block_job_set_speed(const char *device, int64_t 
> > speed, Error **errp)
> >       block_job_set_speed(job, speed, errp);
> >       aio_context_release(aio_context);
> > +    job_unlock();
>
> You add job_unlock(), but not job_lock() ? Something is wrong. And anyway, I 
> thin JOB_LOCK_GUARD / WITH_JOB_LOCK_GUARD are generally safer

Ah, I understand now what's going on. If comment "Returns with job_lock held on 
success" appear in this patch, it would be more obvious.

--
Best regards,
Vladimir