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

[Emanuele Giuseppe Esposito]

Am 07/06/2022 um 17:41 schrieb Paolo Bonzini:
> 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?

Makes sense, thanks.

Emanuele
> 
> Paolo
>