On 2014-10-22 at 13:59, Gonglei wrote:
On 2014/10/22 19:45, Zhang Haoyu wrote:
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 <zhan...@sangfor.com>
Reviewed-by: Max Reitz <mre...@redhat.com>
---
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?
Then this condition is met and you return with -ENOMEM...?
Max
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;
}