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

[Paolo Bonzini <pbonzini@redhat.com>]

On 6/7/22 15:20, Emanuele Giuseppe Esposito wrote:
> 
> 
> 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.

Can you already add "/** Protected by AioContext lock */" in this patch 
and then change it later?

Paolo