Re: [PATCH v6 02/18] job.h: categorize fields in struct Job

[Kevin Wolf <kwolf@redhat.com>]

Am 14.03.2022 um 14:36 hat Emanuele Giuseppe Esposito geschrieben:
> Categorize the fields in struct Job to understand which ones
> need to be protected by the job mutex and which don't.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

I suppose it might be a result of moving things back and forth between
patches, but this patch doesn't really define separate categories.

>  include/qemu/job.h | 59 ++++++++++++++++++++++++++--------------------
>  1 file changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index d1192ffd61..86ec46c09e 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
>   * Long-running operation.
>   */
>  typedef struct Job {
> +
> +    /* Fields set at initialization (job_create), and never modified */

This is clearly a comment starting a category, but I can't see any other
comment indicating that another category would start.

>      /** The ID of the job. May be NULL for internal jobs. */
>      char *id;
>  
> -    /** The type of this job. */
> +    /**
> +     * The type of this job.
> +     * All callbacks are called with job_mutex *not* held.
> +     */
>      const JobDriver *driver;
>  
> -    /** Reference count of the block job */
> -    int refcnt;
> -
> -    /** Current state; See @JobStatus for details. */
> -    JobStatus status;
> -
> -    /** AioContext to run the job coroutine in */
> -    AioContext *aio_context;
> -
>      /**
>       * The coroutine that executes the job.  If not NULL, it is reentered when
>       * busy is false and the job is cancelled.
> +     * Initialized in job_start()
>       */
>      Coroutine *co;
>  
> +    /** True if this job should automatically finalize itself */
> +    bool auto_finalize;
> +
> +    /** True if this job should automatically dismiss itself */
> +    bool auto_dismiss;
> +
> +    /** The completion function that will be called when the job completes.  */
> +    BlockCompletionFunc *cb;
> +
> +    /** The opaque value that is passed to the completion function.  */
> +    void *opaque;
> +
> +    /* ProgressMeter API is thread-safe */
> +    ProgressMeter progress;
> +
> +

And the end of the series, this is where the cutoff is and the rest is:

    /** Protected by job_mutex */

With this in mind, it seems correct to me that everything above progress
is indeed never changed after creating the job. Of course, it's hard to
tell without looking at the final result, so if you have to respin for
some reason, it would be good to mark the end of the section more
clearly for the intermediate state to make sense.

Kevin