[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 20:21:37 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
On 2014/10/22 20:02, Max Reitz wrote:
> On 2014-10-22 at 14:01, Max Reitz wrote:
>> 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 <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?
>>
>> Then this condition is met and you return with -ENOMEM...?
>
> Oh, but I see something different: qemu_try_blockalign() never returns
> NULL, not even when you request 0 bytes.
Yes. But if l1_size2 is zero, will waste memory or some other problems.
Please see below:
the original code:
l1_table = g_try_malloc0(align_offset(0, 512));
-> l1_table = g_try_malloc0(0)
so l1_table == NULL.
after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.
I don't know whether l1_size2 can be zero or not.
Best regards,
-Gonglei
> Therefore, just "if (l1_table == NULL)" is sufficient.
>
> Max
- [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, 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, Max Reitz, 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, 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