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