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

[Kevin Wolf]

Am 13.09.2019 um 16:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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?

Yes, I think that's the right fix.

Kevin