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

2019-09-13 Thread 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 +, 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 
> > ---
> >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



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

2019-09-13 Thread Kevin Wolf
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 +, 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 
> > > > > ---
> > > > >   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



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

2019-09-13 Thread 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 +, 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 
> ---
>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


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

2019-09-13 Thread Maxim Levitsky
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 +, 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 
> > > > ---
> > > >   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.

Best regards,
Maxim Levitsky





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

2019-09-13 Thread Kevin Wolf
Am 13.09.2019 um 16:07 hat Maxim Levitsky geschrieben:
> On Fri, 2019-09-13 at 14:01 +, 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 
> > > ---
> > >   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



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

2019-09-13 Thread Vladimir Sementsov-Ogievskiy
13.09.2019 17:07, Maxim Levitsky wrote:
> On Fri, 2019-09-13 at 14:01 +, 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 
>>> ---
>>>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


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

2019-09-13 Thread Maxim Levitsky
On Fri, 2019-09-13 at 14:01 +, 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 
> > ---
> >   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