Re: [PATCH v3 03/16] job.h: define locked functions

[Paolo Bonzini <pbonzini@redhat.com>]

On 1/5/22 15:01, Emanuele Giuseppe Esposito wrote:
> These functions assume that the job lock is held by the
> caller, to avoid TOC/TOU conditions. Therefore, their
> name must end with _locked.
> 
> Introduce also additional helpers that define _locked
> functions (useful when the job_mutex is globally applied).
> 
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are*nop*.
> 
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>

So, this is the only remaining issue: I am not sure about this rename.
The functions you are changing are

+void job_txn_unref_locked(JobTxn *txn);
+void job_txn_add_job_locked(JobTxn *txn, Job *job);
+void job_ref_locked(Job *job);
+void job_unref_locked(Job *job);
+void job_enter_cond_locked(Job *job, bool(*fn)(Job *job));
+bool job_is_completed_locked(Job *job);
+bool job_is_ready_locked(Job *job);
+void job_pause_locked(Job *job);
+void job_resume_locked(Job *job);
+void job_user_pause_locked(Job *job, Error **errp);
+bool job_user_paused_locked(Job *job);
+void job_user_resume_locked(Job *job, Error **errp);
+Job *job_next_locked(Job *job);
+Job *job_get_locked(const char *id);
+int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp);
+void job_early_fail_locked(Job *job);
+void job_complete_locked(Job *job, Error **errp);
+void job_cancel_locked(Job *job, bool force);
+void job_user_cancel_locked(Job *job, bool force, Error **errp);
+int job_cancel_sync_locked(Job *job, bool force);
+int job_complete_sync_locked(Job *job, Error **errp);
+void job_finalize_locked(Job *job, Error **errp);
+void job_dismiss_locked(Job **job, Error **errp);
+int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp),

and most of them (if not all?) will never be called by the job driver, only
by the monitor.  The two APIs (for driver / for monitor) are quite separate
and have different locking policies: the monitor needs to take the lock to
avoid TOC/TOU races, the driver generally can let the API take the lock.

The rename makes the monitor code heavier, but if you don't do the rename the
functions in job.c are named very inconsistently.  So I'm inclined to say
this patch is fine---but I'd like to hear from others as well.

I think the two APIs should be in two different header files, similar
to how you did the graph/IO split.

Paolo