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

[Maxim Levitsky <mlevitsk@redhat.com>]

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?
Best regards,
	Maxim Levitsky