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

[Emanuele Giuseppe Esposito]

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