| Message ID | 20210707165813.55361-4-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On Wed, Jul 07, 2021 at 06:58:10PM +0200, Emanuele Giuseppe Esposito wrote: > @@ -406,15 +410,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, > error_setg(errp, "Invalid job ID '%s'", job_id); > return NULL; > } > - if (job_get(job_id)) { > - error_setg(errp, "Job ID '%s' already in use", job_id); > - return NULL; > - } > } else if (!(flags & JOB_INTERNAL)) { > error_setg(errp, "An explicit job ID is required"); > return NULL; > } > > + job_lock(); > + if (job_get(job_id)) { > + error_setg(errp, "Job ID '%s' already in use", job_id); > + job_unlock(); > + return NULL; > + } > + Where is the matching job_unlock() in the success case? Please consider lock guard macros like QEMU_LOCK_GUARD()/WITH_QEMU_LOCK_GUARD() to prevent common errors.
On 08/07/2021 12:55, Stefan Hajnoczi wrote: > On Wed, Jul 07, 2021 at 06:58:10PM +0200, Emanuele Giuseppe Esposito wrote: >> @@ -406,15 +410,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, >> error_setg(errp, "Invalid job ID '%s'", job_id); >> return NULL; >> } >> - if (job_get(job_id)) { >> - error_setg(errp, "Job ID '%s' already in use", job_id); >> - return NULL; >> - } >> } else if (!(flags & JOB_INTERNAL)) { >> error_setg(errp, "An explicit job ID is required"); >> return NULL; >> } >> >> + job_lock(); >> + if (job_get(job_id)) { >> + error_setg(errp, "Job ID '%s' already in use", job_id); >> + job_unlock(); >> + return NULL; >> + } >> + > > Where is the matching job_unlock() in the success case? Please consider > lock guard macros like QEMU_LOCK_GUARD()/WITH_QEMU_LOCK_GUARD() to > prevent common errors. > Again this is a dumb mistake, the job_lock/unlock lines should go on patch 5, not here. Apologies. I did not use QEMU_LOCK_GUARD()/WITH_QEMU_LOCK_GUARD() yet because I added some assertions to make sure I also didn't create nested locking situations. The final version will certainly use them. Emanuele
On Wed, Jul 07, 2021 at 06:58:10PM +0200, Emanuele Giuseppe Esposito wrote: > Check for NULL id to job_get, so that in the next patch we can > move job_get inside a single critical section of job_create. > > Also add missing notifier_list_init for the on_idle NotifierList, > which seems to have been forgot. forgotten > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > job.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/job.c b/job.c index 96fb8e9730..48b304c3ff 100644 --- a/job.c +++ b/job.c @@ -375,6 +375,10 @@ Job *job_get(const char *id) { Job *job; + if (!id) { + return NULL; + } + QLIST_FOREACH(job, &jobs, job_list) { if (job->id && !strcmp(id, job->id)) { return job; @@ -406,15 +410,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, error_setg(errp, "Invalid job ID '%s'", job_id); return NULL; } - if (job_get(job_id)) { - error_setg(errp, "Job ID '%s' already in use", job_id); - return NULL; - } } else if (!(flags & JOB_INTERNAL)) { error_setg(errp, "An explicit job ID is required"); return NULL; } + job_lock(); + if (job_get(job_id)) { + error_setg(errp, "Job ID '%s' already in use", job_id); + job_unlock(); + return NULL; + } + job = g_malloc0(driver->instance_size); job->driver = driver; job->id = g_strdup(job_id); @@ -434,6 +441,7 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, notifier_list_init(&job->on_finalize_completed); notifier_list_init(&job->on_pending); notifier_list_init(&job->on_ready); + notifier_list_init(&job->on_idle); job_state_transition(job, JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
Check for NULL id to job_get, so that in the next patch we can move job_get inside a single critical section of job_create. Also add missing notifier_list_init for the on_idle NotifierList, which seems to have been forgot. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- job.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)