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

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

Am 03/06/2022 um 18:00 schrieb Kevin Wolf:
> 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.

How can I do that? I left two empty lines in this patch, I don't know
what to use to signal the end of this category.

Emanuele