Re: [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: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. Therefore, just "if (l1_table 
== NULL)" is sufficient.

Max