Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

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