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

[Max Reitz <mreitz@redhat.com>]

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 <zhanghy@sangfor.com>
>>>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>>>> ---
>>>>>>> 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)".

Max