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

[Emanuele Giuseppe Esposito]

Am 03/06/2022 um 18:17 schrieb 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.
> 

Makes sense, I will switch to the same format as job_is_cancelled_locked().

Emanuele

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