Re: [RFC PATCH v2 03/14] job.h: define locked functions

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 16/12/2021 18:11, Vladimir Sementsov-Ogievskiy wrote:
> 16.12.2021 19:48, Stefan Hajnoczi wrote:
>> On Thu, Nov 04, 2021 at 10:53:23AM -0400, Emanuele Giuseppe Esposito 
>> wrote:
>>>   /** Returns whether the job is ready to be completed. */
>>>   bool job_is_ready(Job *job);
>>> +/** Same as job_is_ready(), but assumes job_lock is held. */
>>> +bool job_is_ready_locked(Job *job);
>>
>> What I see here is that some functions assume job_lock is held but don't
>> have _locked in their name (job_ref()), some assume job_lock is held and
>> have _locked in their name (job_is_ready_locked()), and some assume
>> job_lock is not held (job_is_ready()).
>>
>> That means when _locked is not in the name I don't know whether this
>> function requires job_lock or will deadlock if called under job_lock.
>>
>> Two ways to it obvious:
>>
>> 1. Always have _locked in the name if the function requires job_lock.
>>     Functions without _locked must not be called under job_lock.
>>
>> 2. Don't change the name but use the type system instead:
>>
>>     /*
>>      * Define a unique type so the compiler warns us. It's just a pointer
>>      * so it can be efficiently passed by value.
>>      */
>>     typedef struct { Job *job; } LockedJob;
>>
>>     LockedJob job_lock(Job *job);
>>     Job *job_unlock(LockedJob job);
>>
>>     Now the compiler catches mistakes:
>>
>>     bool job_is_completed(LockedJob job);
>>     bool job_is_ready(Job *job);
>>
>>     Job *j;
>>     LockedJob l;
>>     job_is_completed(j) -> compiler error
>>     job_is_completed(l) -> ok
>>     job_is_ready(j) -> ok
>>     job_is_ready(l) -> compiler error
>>
>>     This approach assumes per-Job locks but a similar API is possible
>>     with a global job_mutex too. There just needs to be a function to
>>     turn Job * into LockedJob and LockedJob back into Job*.
>>
>>     This is slightly exotic. It's not an approach I've seen used in C, so
>>     it's not idiomatic and people might find it unfamiliar.
> 
> Oh yes. If we need something, I'd prefer function renaming.

Ok, I will go with option 1.

> 
>>
>> These are just ideas. If you want to keep it the way it is, that's okay
>> too (although a little confusing).
>>
>>> diff --git a/job.c b/job.c
>>> index 0e4dacf028..e393c1222f 100644
>>> --- a/job.c
>>> +++ b/job.c
>>> @@ -242,7 +242,8 @@ bool job_cancel_requested(Job *job)
>>>       return job->cancelled;
>>>   }
>>> -bool job_is_ready(Job *job)
>>> +/* Called with job_mutex held. */
>>
>> This information should go with the doc comments (and it's already there
>> in job.h!). There is no rule on where to put doc comments but in this
>> case you already added them to job.h, so they are not needed here in
>> job.c. Leaving them could confuse other people into adding doc comments
>> into job.c instead of job.h.
>>

Yes, in general I will leave the comment for static functions in job.c 
and make sure the public ones are only documented in job.h.

Thank you,
Emanuele