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

[Kevin Wolf <kwolf@redhat.com>]

Am 13.09.2019 um 16:37 hat Maxim Levitsky geschrieben:
> 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 <mlevitsk@redhat.com>
> > > > > ---
> > > > >   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.

Fine with me, as long as the fix is kept separate from the refactoring.

Kevin