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

[Vladimir Sementsov-Ogievskiy]

13.09.2019 17:07, Maxim Levitsky wrote:
> 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.

And this is really great! I'm sorry that I've broken these things, we actually 
don't
use encryption (don't ask me why I've implemented threaded encryption :), so, I 
hope
you are not only damaged by my patches but also have some benefit.

> 
> What do you think that I fold these two patches together after all?

OK for me (but I'm not a maintainer).


-- 
Best regards,
Vladimir