Received: from eggs.gnu.org ([2001:4830:134:3::10]:41984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgui0-0005BA-UA for qemu-devel@nongnu.org; Wed, 22 Oct 2014 08:08:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xguhv-0005S1-Q0 for qemu-devel@nongnu.org; Wed, 22 Oct 2014 08:08:08 -0400 Date: Wed, 22 Oct 2014 20:07:53 +0800 From: "Zhang Haoyu" References: <201410221945053926307@sangfor.com>, <54479C3E.1070803@huawei.com> Message-ID: <201410222007512815199@sangfor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: qemu-trivial , Kevin Wolf , qemu-devel , Stefan Hajnoczi , Max Reitz >> Use local variable to bdrv_pwrite_sync L1 table, >> needless to make conversion of cached L1 table between >> big-endian and host style. >> >> Signed-off-by: Zhang Haoyu >> Reviewed-by: Max Reitz >> --- >> v3 -> v4: >> - convert local L1 table to host-style before copy it >> back to s->l1_table >> >> v2 -> v3: >> - replace g_try_malloc0 with qemu_try_blockalign >> - copy the latest local L1 table back to s->l1_table >> after successfully bdrv_pwrite_sync L1 table >> >> v1 -> v2: >> - remove the superflous assignment, l1_table = NULL; >> - replace 512 with BDRV_SECTOR_SIZE, and align_offset with ROUND_UP >> - remove needless check of if (l1_table) before g_free(l1_table) >> >> block/qcow2-refcount.c | 28 ++++++++++++---------------- >> 1 file changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 2bcaaf9..3e4050a 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -881,14 +881,17 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> { >> BDRVQcowState *s = bs->opaque; >> uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2; >> - bool l1_allocated = false; >> int64_t old_offset, old_l2_offset; >> int i, j, l1_modified = 0, nb_csectors, refcount; >> int ret; >> >> l2_table = NULL; >> - l1_table = NULL; >> l1_size2 = l1_size * sizeof(uint64_t); >> + l1_table = qemu_try_blockalign(bs->file, l1_size2); >> + if (l1_size2 && l1_table == NULL) { > >I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, >What will happen? > If the condition of "if (l1_size2 && l1_table == NULL)" is true, below code will be performed, s->cache_discards = false; qcow2_process_discards(bs, ret); g_free(l1_table); What's your question? Thanks, Zhang Haoyu >Best regards, >-Gonglei > >> + ret = -ENOMEM; >> + goto fail; >> + } >> >> s->cache_discards = true; >> >> @@ -896,13 +899,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> * l1_table_offset when it is the current s->l1_table_offset! Be careful >> * when changing this! */ >> if (l1_table_offset != s->l1_table_offset) { >> - l1_table = g_try_malloc0(align_offset(l1_size2, 512)); >> - if (l1_size2 && l1_table == NULL) { >> - ret = -ENOMEM; >> - goto fail; >> - } >> - l1_allocated = true; >> - >> ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2); >> if (ret < 0) { >> goto fail; >> @@ -912,8 +908,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >> be64_to_cpus(&l1_table[i]); >> } else { >> assert(l1_size == s->l1_size); >> - l1_table = s->l1_table; >> - l1_allocated = false; >> + memcpy(l1_table, s->l1_table, l1_size2); >> } >> >> for(i = 0; i < l1_size; i++) { >> @@ -1055,13 +1050,14 @@ fail: >> } >> >> ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1_size2); >> - >> - for (i = 0; i < l1_size; i++) { >> - be64_to_cpus(&l1_table[i]); >> + if (ret == 0) { >> + for (i = 0; i < l1_size; i++) { >> + be64_to_cpus(&l1_table[i]); >> + } >> + memcpy(s->l1_table, l1_table, l1_size2); >> } >> } >> - if (l1_allocated) >> - g_free(l1_table); >> + g_free(l1_table); >> return ret; >> } >>