[Qemu-devel] [PATCH v2 0/2] qcow2: Fix return path of realloc_refcount_block()

[Qemu-devel] [PATCH v2 0/2] qcow2: Fix return path of realloc_refcount_block()

[Max Reitz]

This series fixes the fail path of the static function
realloc_refcount_block() in block/qcow2-refcount.c which is used for
repairing corrupted image files. While doing so, the whole return path
is cleaned up and the comment describing the return value in case of
success is fixed.


Max Reitz (2):
  qcow2: Correct comment for realloc_refcount_block()
  qcow2: Fix fail path in realloc_refcount_block()

 block/qcow2-refcount.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/2] qcow2: Correct comment for realloc_refcount_block()

[Max Reitz]

Contrary to the comment describing this function's behavior, it does not
return 0 on success, but rather the offset of the newly allocated
cluster. This patch adjusts the comment accordingly to reflect the
actual behavior.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6151148..3f2ed08 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1383,7 +1383,7 @@ static int write_reftable_entry(BlockDriverState *bs, int rt_index)
  * does _not_ decrement the reference count for the currently occupied cluster.
  *
  * This function prints an informative message to stderr on error (and returns
- * -errno); on success, 0 is returned.
+ * -errno); on success, the offset of the newly allocated cluster is returned.
  */
 static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
                                       uint64_t offset)
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 1/2] qcow2: Correct comment for realloc_refcount_block()

[Laszlo Ersek]

On 03/17/14 23:04, Max Reitz wrote:
> Contrary to the comment describing this function's behavior, it does not
> return 0 on success, but rather the offset of the newly allocated
> cluster. This patch adjusts the comment accordingly to reflect the
> actual behavior.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 6151148..3f2ed08 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1383,7 +1383,7 @@ static int write_reftable_entry(BlockDriverState *bs, int rt_index)
>   * does _not_ decrement the reference count for the currently occupied cluster.
>   *
>   * This function prints an informative message to stderr on error (and returns
> - * -errno); on success, 0 is returned.
> + * -errno); on success, the offset of the newly allocated cluster is returned.
>   */
>  static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>                                        uint64_t offset)
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH v2 2/2] qcow2: Fix fail path in realloc_refcount_block()

[Max Reitz]

If qcow2_alloc_clusters() fails, new_offset and ret will both be
negative after the fail label, thus passing the first if condition and
subsequently resulting in a call of qcow2_free_clusters() with an
invalid (negative) offset parameter. Fix this by introducing a new label
"fail_free_cluster" which is only invoked if new_offset is indeed
pointing to a newly allocated cluster that should be cleaned up by
freeing it.

While we're at it, clean up the whole fail path. qcow2_cache_put()
should (and actually can) never fail, hence the return value can safely
be ignored (aside from asserting that it indeed did not fail).

Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.

Ultimately, rename the "fail" label to "done", as it is invoked both on
failure and success.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
---
 block/qcow2-refcount.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3f2ed08..4a2df5f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1399,14 +1399,14 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
         fprintf(stderr, "Could not allocate new cluster: %s\n",
                 strerror(-new_offset));
         ret = new_offset;
-        goto fail;
+        goto done;
     }
 
     /* fetch current refcount block content */
     ret = qcow2_cache_get(bs, s->refcount_block_cache, offset, &refcount_block);
     if (ret < 0) {
         fprintf(stderr, "Could not fetch refcount block: %s\n", strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
     /* new block has not yet been entered into refcount table, therefore it is
@@ -1417,8 +1417,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
                 "check failed: %s\n", strerror(-ret));
         /* the image will be marked corrupt, so don't even attempt on freeing
          * the cluster */
-        new_offset = 0;
-        goto fail;
+        goto done;
     }
 
     /* write to new block */
@@ -1426,7 +1425,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
             s->cluster_sectors);
     if (ret < 0) {
         fprintf(stderr, "Could not write refcount block: %s\n", strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
     /* update refcount table */
@@ -1436,24 +1435,27 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
     if (ret < 0) {
         fprintf(stderr, "Could not update refcount table: %s\n",
                 strerror(-ret));
-        goto fail;
+        goto fail_free_cluster;
     }
 
-fail:
-    if (new_offset && (ret < 0)) {
-        qcow2_free_clusters(bs, new_offset, s->cluster_size,
-                QCOW2_DISCARD_ALWAYS);
-    }
+    goto done;
+
+fail_free_cluster:
+    qcow2_free_clusters(bs, new_offset, s->cluster_size, QCOW2_DISCARD_OTHER);
+
+done:
     if (refcount_block) {
-        if (ret < 0) {
-            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
-        } else {
-            ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
-        }
+        /* This should never fail, as it would only do so if the given refcount
+         * block cannot be found in the cache. As this is impossible as long as
+         * there are no bugs, assert the success. */
+        int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
+        assert(tmp == 0);
     }
+
     if (ret < 0) {
         return ret;
     }
+
     return new_offset;
 }
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 2/2] qcow2: Fix fail path in realloc_refcount_block()

[Laszlo Ersek]

On 03/17/14 23:04, Max Reitz wrote:
> If qcow2_alloc_clusters() fails, new_offset and ret will both be
> negative after the fail label, thus passing the first if condition and
> subsequently resulting in a call of qcow2_free_clusters() with an
> invalid (negative) offset parameter. Fix this by introducing a new label
> "fail_free_cluster" which is only invoked if new_offset is indeed
> pointing to a newly allocated cluster that should be cleaned up by
> freeing it.
> 
> While we're at it, clean up the whole fail path. qcow2_cache_put()
> should (and actually can) never fail, hence the return value can safely
> be ignored (aside from asserting that it indeed did not fail).
> 
> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.
> 
> Ultimately, rename the "fail" label to "done", as it is invoked both on
> failure and success.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  block/qcow2-refcount.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3f2ed08..4a2df5f 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1399,14 +1399,14 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>          fprintf(stderr, "Could not allocate new cluster: %s\n",
>                  strerror(-new_offset));
>          ret = new_offset;
> -        goto fail;
> +        goto done;
>      }
>  
>      /* fetch current refcount block content */
>      ret = qcow2_cache_get(bs, s->refcount_block_cache, offset, &refcount_block);
>      if (ret < 0) {
>          fprintf(stderr, "Could not fetch refcount block: %s\n", strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
>      /* new block has not yet been entered into refcount table, therefore it is
> @@ -1417,8 +1417,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>                  "check failed: %s\n", strerror(-ret));
>          /* the image will be marked corrupt, so don't even attempt on freeing
>           * the cluster */
> -        new_offset = 0;
> -        goto fail;
> +        goto done;
>      }
>  
>      /* write to new block */
> @@ -1426,7 +1425,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>              s->cluster_sectors);
>      if (ret < 0) {
>          fprintf(stderr, "Could not write refcount block: %s\n", strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
>      /* update refcount table */
> @@ -1436,24 +1435,27 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>      if (ret < 0) {
>          fprintf(stderr, "Could not update refcount table: %s\n",
>                  strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
> -fail:
> -    if (new_offset && (ret < 0)) {
> -        qcow2_free_clusters(bs, new_offset, s->cluster_size,
> -                QCOW2_DISCARD_ALWAYS);
> -    }
> +    goto done;
> +
> +fail_free_cluster:
> +    qcow2_free_clusters(bs, new_offset, s->cluster_size, QCOW2_DISCARD_OTHER);
> +
> +done:
>      if (refcount_block) {
> -        if (ret < 0) {
> -            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
> -        } else {
> -            ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
> -        }
> +        /* This should never fail, as it would only do so if the given refcount
> +         * block cannot be found in the cache. As this is impossible as long as
> +         * there are no bugs, assert the success. */
> +        int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
> +        assert(tmp == 0);
>      }
> +
>      if (ret < 0) {
>          return ret;
>      }
> +
>      return new_offset;
>  }
>  
> 

I liked the other version better. Especially crossing the trajectories
of the gotos (where "new_offset" used to be zeroed) makes me want to
avert my eyes. However, I've obsessed enough; the code seems correct.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 2/2] qcow2: Fix fail path in realloc_refcount_block()

[Kevin Wolf]

Am 17.03.2014 um 23:04 hat Max Reitz geschrieben:
> If qcow2_alloc_clusters() fails, new_offset and ret will both be
> negative after the fail label, thus passing the first if condition and
> subsequently resulting in a call of qcow2_free_clusters() with an
> invalid (negative) offset parameter. Fix this by introducing a new label
> "fail_free_cluster" which is only invoked if new_offset is indeed
> pointing to a newly allocated cluster that should be cleaned up by
> freeing it.
> 
> While we're at it, clean up the whole fail path. qcow2_cache_put()
> should (and actually can) never fail, hence the return value can safely
> be ignored (aside from asserting that it indeed did not fail).
> 
> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.
> 
> Ultimately, rename the "fail" label to "done", as it is invoked both on
> failure and success.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  block/qcow2-refcount.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 3f2ed08..4a2df5f 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1399,14 +1399,14 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>          fprintf(stderr, "Could not allocate new cluster: %s\n",
>                  strerror(-new_offset));
>          ret = new_offset;
> -        goto fail;
> +        goto done;
>      }
>  
>      /* fetch current refcount block content */
>      ret = qcow2_cache_get(bs, s->refcount_block_cache, offset, &refcount_block);
>      if (ret < 0) {
>          fprintf(stderr, "Could not fetch refcount block: %s\n", strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
>      /* new block has not yet been entered into refcount table, therefore it is
> @@ -1417,8 +1417,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>                  "check failed: %s\n", strerror(-ret));
>          /* the image will be marked corrupt, so don't even attempt on freeing
>           * the cluster */
> -        new_offset = 0;
> -        goto fail;
> +        goto done;
>      }
>  
>      /* write to new block */
> @@ -1426,7 +1425,7 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>              s->cluster_sectors);
>      if (ret < 0) {
>          fprintf(stderr, "Could not write refcount block: %s\n", strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
>      /* update refcount table */
> @@ -1436,24 +1435,27 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>      if (ret < 0) {
>          fprintf(stderr, "Could not update refcount table: %s\n",
>                  strerror(-ret));
> -        goto fail;
> +        goto fail_free_cluster;
>      }
>  
> -fail:
> -    if (new_offset && (ret < 0)) {
> -        qcow2_free_clusters(bs, new_offset, s->cluster_size,
> -                QCOW2_DISCARD_ALWAYS);
> -    }
> +    goto done;

I generally prefer an explicit ret = 0 in such places, so that we don't
have to rely on the next person adding new code before the goto to
understand the dependency here.

But it's correct code, so I won't block it because of this.

Kevin

Re: [Qemu-devel] [PATCH v2 0/2] qcow2: Fix return path of realloc_refcount_block()

[Kevin Wolf]

Am 17.03.2014 um 23:04 hat Max Reitz geschrieben:
> This series fixes the fail path of the static function
> realloc_refcount_block() in block/qcow2-refcount.c which is used for
> repairing corrupted image files. While doing so, the whole return path
> is cleaned up and the comment describing the return value in case of
> success is fixed.
> 
> 
> Max Reitz (2):
>   qcow2: Correct comment for realloc_refcount_block()
>   qcow2: Fix fail path in realloc_refcount_block()
> 
>  block/qcow2-refcount.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)

Thanks, applied to the block branch.

Kevin