[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable
From: |
Gonglei |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table |
Date: |
Wed, 22 Oct 2014 19:59:58 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
> 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?
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;
> }
>
- [Qemu-trivial] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Zhang Haoyu, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table,
Gonglei <=
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table, Zhang Haoyu, 2014/10/22