Re: [PATCH 08/10] qcow2-refcount: improve style of check_refcounts_l1()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

04.05.2021 22:53, Eric Blake wrote:
> On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>   - use g_autofree for l1_table
>>   - better name for size in bytes variable
>>   - reduce code blocks nesting
>>   - whitespaces, braces, newlines
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-refcount.c | 97 +++++++++++++++++++++---------------------
>>   1 file changed, 49 insertions(+), 48 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 44fc0dd5dc..eb6de3dabd 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs,
>>                                 int flags, BdrvCheckMode fix, bool active)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>> -    uint64_t *l1_table = NULL, l2_offset, l1_size2;
>> +    size_t l1_size_bytes = l1_size * L1E_SIZE;
>> +    g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes);
> 
> Note that this now happens...
> 
>> +    uint64_t l2_offset;
>>       int i, ret;
>>   
>> -    l1_size2 = l1_size * L1E_SIZE;
>> +    if (!l1_size) {
>> +        return 0;
> 
> ...before you validate whether l1_size is non-zero, which can result in
> g_try_malloc(0).  Probably harmless, but it might be better if you declare
>   g_autofree uint64_t *l1_table = NULL;
> and then initialize it via malloc only after the sanity check.
> 

Thinking a bit, another thing I don't like is that check if (l1_table == NULL) doesn't immediately follow g_try_malloc() call. So, will move it.

-- 
Best regards,
Vladimir