diff mbox series

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

Message ID 20210504152023.322862-9-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series qcow2 check: check some reserved bits and subcluster bitmaps | expand

Commit Message

Vladimir Sementsov-Ogievskiy May 4, 2021, 3:20 p.m. UTC
- 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(-)

Comments

Eric Blake May 4, 2021, 7:53 p.m. UTC | #1
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.
Vladimir Sementsov-Ogievskiy May 5, 2021, 6:34 a.m. UTC | #2
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.
diff mbox series

Patch

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);
+    uint64_t l2_offset;
     int i, ret;
 
-    l1_size2 = l1_size * L1E_SIZE;
+    if (!l1_size) {
+        return 0;
+    }
 
     /* Mark L1 table as used */
     ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
-                                   l1_table_offset, l1_size2);
+                                   l1_table_offset, l1_size_bytes);
     if (ret < 0) {
-        goto fail;
+        return ret;
+    }
+
+    if (l1_table == NULL) {
+        res->check_errors++;
+        return -ENOMEM;
     }
 
     /* Read L1 table entries from disk */
-    if (l1_size2 > 0) {
-        l1_table = g_try_malloc(l1_size2);
-        if (l1_table == NULL) {
-            ret = -ENOMEM;
-            res->check_errors++;
-            goto fail;
-        }
-        ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
-        if (ret < 0) {
-            fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
-            res->check_errors++;
-            goto fail;
-        }
-        for(i = 0;i < l1_size; i++)
-            be64_to_cpus(&l1_table[i]);
+    ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size_bytes);
+    if (ret < 0) {
+        fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
+        res->check_errors++;
+        return ret;
+    }
+
+    for (i = 0; i < l1_size; i++) {
+        be64_to_cpus(&l1_table[i]);
     }
 
     /* Do the actual checks */
-    for(i = 0; i < l1_size; i++) {
-        l2_offset = l1_table[i];
-        if (l2_offset) {
-            /* Mark L2 table as used */
-            l2_offset &= L1E_OFFSET_MASK;
-            ret = qcow2_inc_refcounts_imrt(bs, res,
-                                           refcount_table, refcount_table_size,
-                                           l2_offset, s->cluster_size);
-            if (ret < 0) {
-                goto fail;
-            }
+    for (i = 0; i < l1_size; i++) {
+        if (!l1_table[i]) {
+            continue;
+        }
 
-            /* L2 tables are cluster aligned */
-            if (offset_into_cluster(s, l2_offset)) {
-                fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
-                    "cluster aligned; L1 entry corrupted\n", l2_offset);
-                res->corruptions++;
-            }
+        l2_offset = l1_table[i] & L1E_OFFSET_MASK;
 
-            /* Process and check L2 entries */
-            ret = check_refcounts_l2(bs, res, refcount_table,
-                                     refcount_table_size, l2_offset, flags,
-                                     fix, active);
-            if (ret < 0) {
-                goto fail;
-            }
+        /* Mark L2 table as used */
+        ret = qcow2_inc_refcounts_imrt(bs, res,
+                                       refcount_table, refcount_table_size,
+                                       l2_offset, s->cluster_size);
+        if (ret < 0) {
+            return ret;
+        }
+
+        /* L2 tables are cluster aligned */
+        if (offset_into_cluster(s, l2_offset)) {
+            fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
+                "cluster aligned; L1 entry corrupted\n", l2_offset);
+            res->corruptions++;
+        }
+
+        /* Process and check L2 entries */
+        ret = check_refcounts_l2(bs, res, refcount_table,
+                                 refcount_table_size, l2_offset, flags,
+                                 fix, active);
+        if (ret < 0) {
+            return ret;
         }
     }
-    g_free(l1_table);
-    return 0;
 
-fail:
-    g_free(l1_table);
-    return ret;
+    return 0;
 }
 
 /*