In general looks good to me.
On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote:
With "intact" we mean that all job.h functions implicitly
take the lock. Therefore API callers are unmodified.
This means that:
- all static functions become _locked, and call _locked functions
Some static functions don't have _locked prefix.. That's, maybe, not wrong. But
it contradicts with commit message and looks inconsistent.
For example job_started and job_should_pause are similar simple getters,
job_shoud_pause is updated to be _locked, but job_started is not updated..
job_exit, job_co_entry are correct exclusions
- all public functions take the lock internally, and call _locked
functions
may be just, "all public function take the lock internally if needed", as some
public funcitons don't need the lock, like job_txn_new or job_progress_* functions
- all public functions called internally by other functions in job.c will have a
_locked counterpart, to avoid deadlocks (job lock already taken)
counterparts sometimes made public and are unused for now. That's OK, just
mention.
- public functions called only from exernal files (not job.c) do not
have _locked() counterpart and take the lock inside
Some small public functions still don't take the lock inside and don't have
_locked() prefix.
job_is_internal
job_type
job_type_str
job_sleep_timer_cb is static, but it's called only by external source, so it
should take lock inside, like public functions. (and like job_exit)
job_pause_point_locked, job_user_resume_locked, job_complete_locked: need to
mention in a comment, that function may temporarily unlock the mutex
Also, the commennt "realeases it temporarily" should be propagated to all
_locked callers of the function
job_{lock/unlock} is independent from real_job_{lock/unlock}.
Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*
.Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
---
include/qemu/job.h | 73 +++++-
job.c | 607 +++++++++++++++++++++++++++++++--------------
[..]
- AIO_WAIT_WHILE(job->aio_context,
- (job_enter(job), !job_is_completed(job)));
+ job_unlock();
+ AIO_WAIT_WHILE_UNLOCKED(job->aio_context,
+ (job_enter(job), !job_is_completed(job)));
Is it correct here to move to UNLOCKED wait? We don't touch any other
aio-context related things in this patch..
Even if it's OK, it should be in another commit I think.
+ job_lock();
- ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
- job_unref(job);
+ ret = (job_is_cancelled_locked(job) && job->ret == 0)
+ ? -ECANCELED : job->ret;
+ job_unref_locked(job);
return ret;
}
+
+int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error
**errp)
+{
+ JOB_LOCK_GUARD();
+ return job_finish_sync_locked(job, finish, errp);
+}
\ No newline at end of file
No newline at end of file... I think, good to add one)
--
Best regards,
Vladimir