aboutsummaryrefslogtreecommitdiff
path: root/block/qcow2-refcount.c
diff options
context:
space:
mode:
authorMax Reitz <mreitz@redhat.com>2014-03-17 23:04:52 +0100
committerKevin Wolf <kwolf@redhat.com>2014-03-19 09:39:41 +0100
commita134d90f50806597c5da4fd191352fe62d40f71a (patch)
treed395522876aa647f41a62b0457e0be085fa4d52f /block/qcow2-refcount.c
parent8a15b813e6034856d4177c6ab242791795434c15 (diff)
qcow2: Fix fail path in realloc_refcount_block()
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. Suggested-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Diffstat (limited to 'block/qcow2-refcount.c')
-rw-r--r--block/qcow2-refcount.c34
1 files changed, 18 insertions, 16 deletions
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3f2ed08466..4a2df5fb99 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;
}