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.
Emanuele
