Re: [PATCH v2 2/7] block-copy: improve documentation of BlockCopyTask and BlockCopyState types and functions

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 20/05/2021 17:00, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
>> As done in BlockCopyCallState, categorize BlockCopyTask
>> and BlockCopyState in IN, State and OUT fields.
>> This is just to understand which field has to be protected with a lock.
>>
>> BlockCopyTask .zeroes is a special case, because it is only initialized
>> and then read by the coroutine in block_copy_task_entry.
>>
>> Also set block_copy_task_create as coroutine_fn because:
>> 1) it is static and only invoked by coroutine functions
>> 2) next patches will introduce and use a CoMutex lock there
> 
> this change is unrelated, why not to put it into commit, which adds use 
> of CoMutex in that function?

Ok I will move it in patch 4.

> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

[...]
> 
> you add an empty line before group, it looks good
> 
>> +    /* State */
>>       int64_t in_flight_bytes;
>> -    int64_t cluster_size;
>>       BlockCopyMethod method;
>> -    int64_t max_transfer;
>> -    uint64_t len;
>>       QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all 
>> block-copy calls */
>>       QLIST_HEAD(, BlockCopyCallState) calls;
> 
> but not here..

Because these are still State fields, so in the same State group. It is 
a different sub-category.

> 
>> +    /* State fields that use a thread-safe API */
>> +    BdrvDirtyBitmap *copy_bitmap;
>> +    ProgressMeter *progress;
>> +    SharedResource *mem;
>> +    RateLimit rate_limit;
>> +    /*
>> +     * IN parameters. Initialized in block_copy_state_new()
>> +     * and never changed.
>> +     */
>> +    int64_t cluster_size;
>> +    int64_t max_transfer;
>> +    uint64_t len;
>>       BdrvRequestFlags write_flags;
>>       /*

Thank you,
Emanuele