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

2021-05-05 Thread Vladimir Sementsov-Ogievskiy

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 
---
  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



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

2021-05-04 Thread Eric Blake
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 
> ---
>  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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




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

2021-05-04 Thread Vladimir Sementsov-Ogievskiy
 - 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 
---
 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);
+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(_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(_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;
 }
 
 /*
-- 
2.29.2