Re: [PATCH v6 05/18] job.h: add _locked duplicates for job API functions called with and without job_mutex

[Kevin Wolf]

Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
> In preparation to the job_lock/unlock usage, create _locked
> duplicates of some functions, since they will be sometimes called with
> job_mutex held (mostly within job.c),
> and sometimes without (mostly from JobDrivers using the job API).
> 
> Therefore create a _locked version of such function, so that it
> can be used in both cases.
> 
> List of functions duplicated as _locked:
> job_is_ready (both versions are public)
> job_is_completed (both versions are public)
> job_is_cancelled (_locked version is public, needed by mirror.c)
> job_pause_point (_locked version is static, purely done to simplify the code)
> job_cancel_requested (_locked version is static)
> 
> 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 | 25 +++++++++++++++++++++---
>  job.c              | 48 ++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 64 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 6000463126..aa33d091b1 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -473,21 +473,40 @@ const char *job_type_str(const Job *job);
>  /** Returns true if the job should not be visible to the management layer. */
>  bool job_is_internal(Job *job);
>  
> -/** Returns whether the job is being cancelled. */
> +/**
> + * Returns whether the job is being cancelled.
> + * Called with job_mutex *not* held.
> + */
>  bool job_is_cancelled(Job *job);
>  
> +/** Just like job_is_cancelled, but called between job_lock and job_unlock */
> +bool job_is_cancelled_locked(Job *job);
> +
>  /**
>   * Returns whether the job is scheduled for cancellation (at an
>   * indefinite point).
> + * Called with job_mutex *not* held.
>   */
>  bool job_cancel_requested(Job *job);
>  
> -/** Returns whether the job is in a completed state. */
> +/**
> + * Returns whether the job is in a completed state.
> + * Called with job_mutex *not* held.
> + */
>  bool job_is_completed(Job *job);
>  
> -/** Returns whether the job is ready to be completed. */
> +/** Same as job_is_completed(), but assumes job_lock is held. */
> +bool job_is_completed_locked(Job *job);

Any reason why this comment is phrased differently than for
job_is_cancelled_locked()? I don't mind which one we use, but if they
should express the same thing, it would be better to have the same
wording. If they should express different things, it need to be clearer
what they are.

Also, I assume job_mutex is meant because job_lock() is a function, not
the lock that is held.

> +/**
> + * Returns whether the job is ready to be completed.
> + * Called with job_mutex *not* held.
> + */
>  bool job_is_ready(Job *job);
>  
> +/** Same as job_is_ready(), but assumes job_lock is held. */
> +bool job_is_ready_locked(Job *job);

Same as above.

>  /**
>   * Request @job to pause at the next pause point. Must be paired with
>   * job_resume(). If the job is supposed to be resumed by user action, call

Kevin