update_refcount() and qcow2_update_cluster_refcount() currently take a
signed addend. At least one caller passes a value directly derived from
an absolute refcount that should be reached ("l2_refcount - 1" in
expand_zero_clusters_in_l1()). Therefore, the addend should be unsigned
because unsigned overflow is well-defined in contrast to signed
overflow. This will be especially important for 64 bit refcounts.
Because update_refcount() then no longer knows whether the refcount
should be increased or decreased (which is important for setting the
refblock-L2-table cache dependency and for overflow/underflow checks),
it now requires an additional flag which specified exactly that. The
same applies to qcow2_update_cluster_refcount().
Signed-off-by: Max Reitz <mre...@redhat.com>
---
block/qcow2-cluster.c | 2 +-
block/qcow2-refcount.c | 65 ++++++++++++++++++++++++++++++++------------------
block/qcow2.h | 3 ++-
3 files changed, 45 insertions(+), 25 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 133aad3..b664d2f 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1699,7 +1699,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState
*bs, uint64_t *l1_table,
/* For shared L2 tables, set the refcount accordingly (it
is
* already 1 and needs to be l2_refcount) */
ret = qcow2_update_cluster_refcount(bs,
- offset >> s->cluster_bits, l2_refcount - 1,
+ offset >> s->cluster_bits, l2_refcount - 1, false,
QCOW2_DISCARD_OTHER);
if (ret < 0) {
qcow2_free_clusters(bs, offset, s->cluster_size,
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index bd37b8d..99a3cce 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -29,8 +29,8 @@
static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
- int64_t offset, int64_t length,
- int addend, enum qcow2_discard_type type);
+ int64_t offset, int64_t length, uint16_t addend,
+ bool decrease, enum qcow2_discard_type type);
/*********************************************************/
@@ -263,7 +263,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
} else {
/* Described somewhere else. This can recurse at most twice before we
* arrive at a block that describes itself. */
- ret = update_refcount(bs, new_block, s->cluster_size, 1,
+ ret = update_refcount(bs, new_block, s->cluster_size, 1, false,
QCOW2_DISCARD_NEVER);
if (ret < 0) {
goto fail_block;
@@ -530,8 +530,14 @@ found:
}
/* XXX: cache several refcount block clusters ? */
+/* @addend is the absolute value of the addend; if @decrease is set, @addend
+ * will be subtracted from the current refcount, otherwise it will be added */
static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
- int64_t offset, int64_t length, int addend, enum qcow2_discard_type type)
+ int64_t offset,
+ int64_t length,
+ uint16_t addend,
+ bool decrease,
+ enum qcow2_discard_type
type)
{
BDRVQcowState *s = bs->opaque;
int64_t start, last, cluster_offset;
@@ -540,8 +546,9 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
int ret;
#ifdef DEBUG_ALLOC2
- fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64 "
addend=%d\n",
- offset, length, addend);
+ fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64
+ " addend=%s%" PRIu16 "\n", offset, length, decrease ? "-" : "",
+ addend);
#endif
if (length < 0) {
return -EINVAL;
@@ -549,7 +556,7 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
return 0;
}
- if (addend < 0) {
+ if (decrease) {
qcow2_cache_set_dependency(bs, s->refcount_block_cache,
s->l2_table_cache);
}
@@ -559,7 +566,8 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
for(cluster_offset = start; cluster_offset <= last;
cluster_offset += s->cluster_size)
{
- int block_index, refcount;
+ int block_index;
+ uint16_t refcount;
int64_t cluster_index = cluster_offset >> s->cluster_bits;
int64_t table_index = cluster_index >> s->refcount_block_bits;
@@ -586,11 +594,18 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
block_index = cluster_index & (s->refcount_block_size - 1);
refcount = be16_to_cpu(refcount_block[block_index]);
- refcount += addend;
- if (refcount < 0 || refcount > s->refcount_max) {
+ if (decrease ? ((uint16_t)(refcount - addend) > refcount)
+ : ((uint16_t)(refcount + addend) < refcount ||
+ (uint16_t)(refcount + addend) > s->refcount_max))
+ {
ret = -EINVAL;
goto fail;
}
+ if (decrease) {
+ refcount -= addend;
+ } else {
+ refcount += addend;
+ }
if (refcount == 0 && cluster_index < s->free_cluster_index) {
s->free_cluster_index = cluster_index;
}
@@ -623,8 +638,8 @@ fail:
*/
if (ret < 0) {
int dummy;
- dummy = update_refcount(bs, offset, cluster_offset - offset, -addend,
- QCOW2_DISCARD_NEVER);
+ dummy = update_refcount(bs, offset, cluster_offset - offset, addend,
+ !decrease, QCOW2_DISCARD_NEVER);
(void)dummy;
}
@@ -634,18 +649,21 @@ fail:
/*
* Increases or decreases the refcount of a given cluster.
*
+ * @addend is the absolute value of the addend; if @decrease is set, @addend
+ * will be subtracted from the current refcount, otherwise it will be added.
+ *
* On success 0 is returned; on failure -errno is returned.
*/
int qcow2_update_cluster_refcount(BlockDriverState *bs,
int64_t cluster_index,
- int addend,
+ uint16_t addend, bool decrease,
enum qcow2_discard_type type)
{
BDRVQcowState *s = bs->opaque;
int ret;
ret = update_refcount(bs, cluster_index << s->cluster_bits, 1, addend,
- type);
+ decrease, type);
if (ret < 0) {
return ret;
}
@@ -709,7 +727,7 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t
size)
return offset;
}
- ret = update_refcount(bs, offset, size, 1, QCOW2_DISCARD_NEVER);
+ ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER);
} while (ret == -EAGAIN);
if (ret < 0) {
@@ -746,7 +764,7 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t
offset,
}
/* And then allocate them */
- ret = update_refcount(bs, offset, i << s->cluster_bits, 1,
+ ret = update_refcount(bs, offset, i << s->cluster_bits, 1, false,
QCOW2_DISCARD_NEVER);
} while (ret == -EAGAIN);
@@ -786,7 +804,7 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
s->free_byte_offset = 0;
if (offset_into_cluster(s, offset) != 0)
qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
- QCOW2_DISCARD_NEVER);
+ false, QCOW2_DISCARD_NEVER);
} else {
offset = qcow2_alloc_clusters(bs, s->cluster_size);
if (offset < 0) {
@@ -797,7 +815,7 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
/* we are lucky: contiguous data */
offset = s->free_byte_offset;
qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
- QCOW2_DISCARD_NEVER);
+ false, QCOW2_DISCARD_NEVER);
s->free_byte_offset += size;
} else {
s->free_byte_offset = offset;
@@ -820,7 +838,7 @@ void qcow2_free_clusters(BlockDriverState *bs,
int ret;
BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_FREE);
- ret = update_refcount(bs, offset, size, -1, type);
+ ret = update_refcount(bs, offset, size, 1, true, type);
if (ret < 0) {
fprintf(stderr, "qcow2_free_clusters failed: %s\n", strerror(-ret));
/* TODO Remember the clusters to free them later and avoid leaking */
@@ -950,7 +968,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
if (addend != 0) {
ret = update_refcount(bs,
(offset & s->cluster_offset_mask) & ~511,
- nb_csectors * 512, addend,
+ nb_csectors * 512, abs(addend), addend < 0,
QCOW2_DISCARD_SNAPSHOT);
if (ret < 0) {
goto fail;
@@ -981,7 +999,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
}
if (addend != 0) {
ret = qcow2_update_cluster_refcount(bs,
- cluster_index, addend,
+ cluster_index, abs(addend), addend < 0,
QCOW2_DISCARD_SNAPSHOT);
if (ret < 0) {
goto fail;
@@ -1024,7 +1042,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
if (addend != 0) {
ret = qcow2_update_cluster_refcount(bs, l2_offset >>
s->cluster_bits,
- addend,
+ abs(addend), addend < 0,
QCOW2_DISCARD_SNAPSHOT);
if (ret < 0) {
goto fail;
@@ -1677,7 +1695,8 @@ static void compare_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
if (num_fixed) {
ret = update_refcount(bs, i << s->cluster_bits, 1,
- (int)refcount2 - (int)refcount1,
+ abs(refcount2 - refcount1),
+ refcount1 > refcount2,
QCOW2_DISCARD_ALWAYS);
if (ret >= 0) {
(*num_fixed)++;
diff --git a/block/qcow2.h b/block/qcow2.h
index 1e59277..0240ee8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -493,7 +493,8 @@ int qcow2_get_refcount(BlockDriverState *bs, int64_t
cluster_index,
uint16_t *refcount);
int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index,
- int addend, enum qcow2_discard_type type);
+ uint16_t addend, bool decrease,
+ enum qcow2_discard_type type);
int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size);
int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
--
1.9.3
