[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:30:44 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
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?
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 <=
- 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