Re: [PATCH v9 17/21] blockjob.h: categorize fields in struct BlockJob

[Emanuele Giuseppe Esposito]

Am 11/07/2022 um 16:44 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
>> The same job lock is being used also to protect some of blockjob fields.
>> Categorize them just as done in job.h.
> 
> Thanks!
> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   include/block/blockjob.h | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 8b65d3949d..912e10b083 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -40,10 +40,16 @@ typedef struct BlockJobDriver BlockJobDriver;
>>    * Long-running operation on a BlockDriverState.
>>    */
>>   typedef struct BlockJob {
>> -    /** Data belonging to the generic Job infrastructure */
>> +    /**
>> +     * Data belonging to the generic Job infrastructure.
>> +     * Protected by job mutex.
>> +     */
>>       Job job;
>>   -    /** Status that is published by the query-block-jobs QMP API */
>> +    /**
>> +     * Status that is published by the query-block-jobs QMP API.
>> +     * Protected by job mutex.
>> +     */
>>       BlockDeviceIoStatus iostatus;
>>         /** Speed that was set with @block_job_set_speed.  */
>> @@ -55,6 +61,8 @@ typedef struct BlockJob {
>>       /** Block other operations when block job is running */
>>       Error *blocker;
>>   +    /** All notifiers are set once in block_job_create() and never
>> modified. */
>> +
>>       /** Called when a cancelled job is finalised. */
>>       Notifier finalize_cancelled_notifier;
>>   @@ -70,7 +78,10 @@ typedef struct BlockJob {
>>       /** Called when the job coroutine yields or terminates */
>>       Notifier idle_notifier;
>>   -    /** BlockDriverStates that are involved in this block job */
>> +    /**
>> +     * BlockDriverStates that are involved in this block job.
>> +     * Always modified and read under QEMU global mutex
>> (GLOBAL_STATE_CODE)
>> +     */
>>       GSList *nodes;
>>   } BlockJob;
>>   
> 
> Can we also add GLOBAL_STATE_CODE();  marker into
> child_job_can_set_aio_ctx() and child_job_set_aio_ctx() ?

Usually for callbacks I feel redundant to add assertions there. It is
enough to look at their definition in the header to understand which
category are they in.

Emanuele
> 
> Anyway:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>