Re: [PATCH v8 05/20] job.c: add job_lock/unlock while keeping job.h intact

[Vladimir Sementsov-Ogievskiy]

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 <eesposit@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