[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:38:35 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
On 2014/10/22 20:32, Max Reitz wrote:
> On 2014-10-22 at 14:30, Gonglei wrote:
>> On 2014/10/22 20:24, Max Reitz wrote:
>>
>>> On 2014-10-22 at 14:21, Gonglei wrote:
>>>> 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.
>>> Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".
>>>
>> Hum, sorry for my typo ;)
>>
>>>> I don't know whether l1_size2 can be zero or not.
>>> Probably not, but if it cannot be zero, testing for that case makes even
>>> less sense.
>>>
>> So, can we add a check at the begin of this function?
>
> We can just omit the "l1_size2 && " from the "if (l1_size2 && l1_table
> == NULL)".
>
Ack.
Best regards,
-Gonglei
- [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, 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 <=
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table, Zhang Haoyu, 2014/10/22