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 <mlevi...@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. > > > > [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
