[Qemu-devel] [PATCH v4 05/26] qcow2: Use unsigned addend for update_refcount()

[Max Reitz]

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 <address@hidden>
---
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c | 63 ++++++++++++++++++++++++++++++++------------------
 block/qcow2.h          |  3 ++-
 3 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a8baecb..da37535 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..b3aed9c 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,16 @@ found:
 }
 
 /* XXX: cache several refcount block clusters ? */
+/* In order to decrease refcounts, set @addend to the two's complement (giving 
a
+ * negative value and letting the implicit cast handle it is enough) and set
+ * @decrease to true. @decrease must be false if the refcount should be
+ * increased. */
 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 +548,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=%" PRId16 " decrease=%d\n", offset, length,
+            (int16_t)addend, decrease);
 #endif
     if (length < 0) {
         return -EINVAL;
@@ -549,7 +558,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 +568,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 +596,14 @@ 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 ((uint16_t)(refcount + addend) > s->refcount_max ||
+            (!decrease && (uint16_t)(refcount + addend) < refcount) ||
+            ( decrease && (uint16_t)(refcount + addend) > refcount))
+        {
             ret = -EINVAL;
             goto fail;
         }
+        refcount += addend;
         if (refcount == 0 && cluster_index < s->free_cluster_index) {
             s->free_cluster_index = cluster_index;
         }
@@ -624,7 +637,7 @@ fail:
     if (ret < 0) {
         int dummy;
         dummy = update_refcount(bs, offset, cluster_offset - offset, -addend,
-                                QCOW2_DISCARD_NEVER);
+                                !decrease, QCOW2_DISCARD_NEVER);
         (void)dummy;
     }
 
@@ -634,18 +647,23 @@ fail:
 /*
  * Increases or decreases the refcount of a given cluster.
  *
+ * In order to decrease refcounts, set @addend to the two's complement (giving 
a
+ * negative value and letting the implicit cast handle it is enough) and set
+ * @decrease to true. @decrease must be false if the refcount should be
+ * increased.
+ *
  * 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, 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, 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,
+                                                    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,
+                                      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