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 <eespo...@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
>
