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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]