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

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

On 19/01/2022 11:44, Paolo Bonzini wrote:
> 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.

The split was proposed in previous versions, but Vladimir did not really 
like it and suggested to send it as a separate series:

https://patchew.org/QEMU/20211104153121.1362449-1-eesposit@redhat.com/


Vladimir's comment:
https://patchew.org/QEMU/20211104153121.1362449-1-eesposit@redhat.com/

Thank you,
Emanuele

> 
> Paolo
>