Re: [Qemu-stable] [Qemu-devel] [PATCH v4 1/3] block/qcow2: refactoring of threaded encryption code

[Vladimir Sementsov-Ogievskiy]

13.09.2019 17:37, Maxim Levitsky wrote:
> On Fri, 2019-09-13 at 16:24 +0200, Kevin Wolf wrote:
>> Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
>>> On Fri, 2019-09-13 at 14:01 +0000, Vladimir Sementsov-Ogievskiy wrote:
>>>> 13.09.2019 15:59, Maxim Levitsky wrote:
>>>>> This commit tries to clarify few function arguments,
>>>>> and add comments describing the encrypt/decrypt interface
>>>>>
>>>>> Signed-off-by: Maxim Levitsky <address@hidden>
>>>>> ---
>>>>>    block/qcow2-cluster.c |  9 ++++---
>>>>>    block/qcow2-threads.c | 62 ++++++++++++++++++++++++++++++++++---------
>>>>>    block/qcow2.c         |  5 ++--
>>>>>    block/qcow2.h         |  8 +++---
>>>>>    4 files changed, 61 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>>>> index f09cc992af..46b0854d7e 100644
>>>>> --- a/block/qcow2-cluster.c
>>>>> +++ b/block/qcow2-cluster.c
>>>>> @@ -463,8 +463,8 @@ static int coroutine_fn 
>>>>> do_perform_cow_read(BlockDriverState *bs,
>>>>>    }
>>>>>    
>>>>>    static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>>>>> -                                                uint64_t 
>>>>> src_cluster_offset,
>>>>> -                                                uint64_t cluster_offset,
>>>>> +                                                uint64_t 
>>>>> guest_cluster_offset,
>>>>> +                                                uint64_t 
>>>>> host_cluster_offset,
>>>>>                                                    unsigned 
>>>>> offset_in_cluster,
>>>>>                                                    uint8_t *buffer,
>>>>>                                                    unsigned bytes)
>>>>> @@ -474,8 +474,9 @@ static bool coroutine_fn 
>>>>> do_perform_cow_encrypt(BlockDriverState *bs,
>>>>>            assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>>>>>            assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>>>>>            assert(s->crypto);
>>>>> -        if (qcow2_co_encrypt(bs, cluster_offset,
>>>>> -                             src_cluster_offset + offset_in_cluster,
>>>>> +        if (qcow2_co_encrypt(bs,
>>>>> +                             host_cluster_offset + offset_in_cluster,
>>>>> +                             guest_cluster_offset + offset_in_cluster,
>>>>
>>>> oops, seems you accidentally fixed the bug, which you are going to fix in 
>>>> the next
>>>> patch, as now correct offsets are given to qcow2_co_encrypt :)
>>>>
>>>> and next patch no is a simple no-logic-change refactoring, so at least 
>>>> commit message
>>>> should be changed.
>>>
>>> Yep :-( I am trying my best in addition to fixing the bug, also clarify the 
>>> area to
>>> avoid this from happening again.
>>>
>>> What do you think that I fold these two patches together after all?
>>
>> No, just make sure that your refactoring patch is really just
>> refactoring without semantic change, i.e. make sure to preserve the bug
>> in this patch.
>>
>> Maybe you should actually have two refactoring patches (this one without
>> the addition of offset_in_cluster, and patch 2) and an additional
>> one-liner for the actual fix.
>>
>> Kevin
> 
> Let me do it simplier I'll just split it to one liner patch that fixes it
> and second patch that does all the refactoring.
> 

[me typing actually the same suggestion in parallel, but you were the first]

I think it's the best: firstly fix the bug in a simple patch and then
refactor to make code better.

I expect something like simply
s/cluster_offset/start_of_cluster(cluster_offset + offset_in_cluster) in
qcow2_co_encrypt call from do_perform_cow_encrypt,
yes?

-- 
Best regards,
Vladimir