diff mbox

[v2,08/21] qcow2: More helpers for refcount modification

Message ID 1415970374-16811-9-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Nov. 14, 2014, 1:06 p.m. UTC 
Add helper functions for getting and setting refcounts in a refcount
array for any possible refcount order, and choose the correct one during
refcount initialization.

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

Comments

Eric Blake Nov. 15, 2014, 5:08 p.m. UTC | #1 
On 11/14/2014 06:06 AM, Max Reitz wrote:
> Add helper functions for getting and setting refcounts in a refcount
> array for any possible refcount order, and choose the correct one during
> refcount initialization.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 144 insertions(+), 2 deletions(-)
> 

> 
> +static void get_refcount_functions(int refcount_order,
> +                                   Qcow2GetRefcountFunc **get,
> +                                   Qcow2SetRefcountFunc **set)
> +{
> +    switch (refcount_order) {
> +        case 0:
> +            *get = &get_refcount_ro0;
> +            *set = &set_refcount_ro0;
> +            break;

Bike-shedding: instead of a switch statement and open-coded assignments,
is it worth setting up an array of function pointers where you just grab
the correct functions by doing array[refcount_order]?  But I don't see
any strong reason to change style; what you have works.

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz Nov. 17, 2014, 8:44 a.m. UTC | #2 
On 2014-11-15 at 18:08, Eric Blake wrote:
> On 11/14/2014 06:06 AM, Max Reitz wrote:
>> Add helper functions for getting and setting refcounts in a refcount
>> array for any possible refcount order, and choose the correct one during
>> refcount initialization.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 144 insertions(+), 2 deletions(-)
>>
>> +static void get_refcount_functions(int refcount_order,
>> +                                   Qcow2GetRefcountFunc **get,
>> +                                   Qcow2SetRefcountFunc **set)
>> +{
>> +    switch (refcount_order) {
>> +        case 0:
>> +            *get = &get_refcount_ro0;
>> +            *set = &set_refcount_ro0;
>> +            break;
> Bike-shedding: instead of a switch statement and open-coded assignments,
> is it worth setting up an array of function pointers where you just grab
> the correct functions by doing array[refcount_order]?  But I don't see
> any strong reason to change style; what you have works.

I thought about it, but it wouldn't get much shorter. But maybe it looks 
nicer. I ought to think about it again.

Max

> Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b3ca7d2..2e13a9c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -32,10 +32,73 @@  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
                             int64_t offset, int64_t length,
                             int addend, enum qcow2_discard_type type);
 
+static uint64_t get_refcount_ro0(const void *refcount_array, uint64_t index);
+static uint64_t get_refcount_ro1(const void *refcount_array, uint64_t index);
+static uint64_t get_refcount_ro2(const void *refcount_array, uint64_t index);
+static uint64_t get_refcount_ro3(const void *refcount_array, uint64_t index);
 static uint64_t get_refcount_ro4(const void *refcount_array, uint64_t index);
+static uint64_t get_refcount_ro5(const void *refcount_array, uint64_t index);
+static uint64_t get_refcount_ro6(const void *refcount_array, uint64_t index);
 
+static void set_refcount_ro0(void *refcount_array, uint64_t index,
+                             uint64_t value);
+static void set_refcount_ro1(void *refcount_array, uint64_t index,
+                             uint64_t value);
+static void set_refcount_ro2(void *refcount_array, uint64_t index,
+                             uint64_t value);
+static void set_refcount_ro3(void *refcount_array, uint64_t index,
+                             uint64_t value);
 static void set_refcount_ro4(void *refcount_array, uint64_t index,
                              uint64_t value);
+static void set_refcount_ro5(void *refcount_array, uint64_t index,
+                             uint64_t value);
+static void set_refcount_ro6(void *refcount_array, uint64_t index,
+                             uint64_t value);
+
+static void get_refcount_functions(int refcount_order,
+                                   Qcow2GetRefcountFunc **get,
+                                   Qcow2SetRefcountFunc **set)
+{
+    switch (refcount_order) {
+        case 0:
+            *get = &get_refcount_ro0;
+            *set = &set_refcount_ro0;
+            break;
+
+        case 1:
+            *get = &get_refcount_ro1;
+            *set = &set_refcount_ro1;
+            break;
+
+        case 2:
+            *get = &get_refcount_ro2;
+            *set = &set_refcount_ro2;
+            break;
+
+        case 3:
+            *get = &get_refcount_ro3;
+            *set = &set_refcount_ro3;
+            break;
+
+        case 4:
+            *get = &get_refcount_ro4;
+            *set = &set_refcount_ro4;
+            break;
+
+        case 5:
+            *get = &get_refcount_ro5;
+            *set = &set_refcount_ro5;
+            break;
+
+        case 6:
+            *get = &get_refcount_ro6;
+            *set = &set_refcount_ro6;
+            break;
+
+        default:
+            abort();
+    }
+}
 
 
 /*********************************************************/
@@ -47,8 +110,8 @@  int qcow2_refcount_init(BlockDriverState *bs)
     unsigned int refcount_table_size2, i;
     int ret;
 
-    s->get_refcount = &get_refcount_ro4;
-    s->set_refcount = &set_refcount_ro4;
+    get_refcount_functions(s->refcount_order,
+                           &s->get_refcount, &s->set_refcount);
 
     assert(s->refcount_table_size <= INT_MAX / sizeof(uint64_t));
     refcount_table_size2 = s->refcount_table_size * sizeof(uint64_t);
@@ -80,6 +143,59 @@  void qcow2_refcount_close(BlockDriverState *bs)
 }
 
 
+static uint64_t get_refcount_ro0(const void *refcount_array, uint64_t index)
+{
+    return (((const uint8_t *)refcount_array)[index / 8] >> (index % 8)) & 0x1;
+}
+
+static void set_refcount_ro0(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    assert(!(value >> 1));
+    ((uint8_t *)refcount_array)[index / 8] &= ~(0x1 << (index % 8));
+    ((uint8_t *)refcount_array)[index / 8] |= value << (index % 8);
+}
+
+static uint64_t get_refcount_ro1(const void *refcount_array, uint64_t index)
+{
+    return (((const uint8_t *)refcount_array)[index / 4] >> (2 * (index % 4)))
+           & 0x3;
+}
+
+static void set_refcount_ro1(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    assert(!(value >> 2));
+    ((uint8_t *)refcount_array)[index / 4] &= ~(0x3 << (2 * (index % 4)));
+    ((uint8_t *)refcount_array)[index / 4] |= value << (2 * (index % 4));
+}
+
+static uint64_t get_refcount_ro2(const void *refcount_array, uint64_t index)
+{
+    return (((const uint8_t *)refcount_array)[index / 2] >> (4 * (index % 2)))
+           & 0xf;
+}
+
+static void set_refcount_ro2(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    assert(!(value >> 4));
+    ((uint8_t *)refcount_array)[index / 2] &= ~(0xf << (4 * (index % 2)));
+    ((uint8_t *)refcount_array)[index / 2] |= value << (4 * (index % 2));
+}
+
+static uint64_t get_refcount_ro3(const void *refcount_array, uint64_t index)
+{
+    return ((const uint8_t *)refcount_array)[index];
+}
+
+static void set_refcount_ro3(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    assert(!(value >> 8));
+    ((uint8_t *)refcount_array)[index] = value;
+}
+
 static uint64_t get_refcount_ro4(const void *refcount_array, uint64_t index)
 {
     return be16_to_cpu(((const uint16_t *)refcount_array)[index]);
@@ -92,6 +208,32 @@  static void set_refcount_ro4(void *refcount_array, uint64_t index,
     ((uint16_t *)refcount_array)[index] = cpu_to_be16(value);
 }
 
+static uint64_t get_refcount_ro5(const void *refcount_array, uint64_t index)
+{
+    return be32_to_cpu(((const uint32_t *)refcount_array)[index]);
+}
+
+static void set_refcount_ro5(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    assert(!(value >> 32));
+    ((uint32_t *)refcount_array)[index] = cpu_to_be32(value);
+}
+
+static uint64_t get_refcount_ro6(const void *refcount_array, uint64_t index)
+{
+    return be64_to_cpu(((const uint64_t *)refcount_array)[index]);
+}
+
+static void set_refcount_ro6(void *refcount_array, uint64_t index,
+                             uint64_t value)
+{
+    /* for 64 bit refcounts, refcount_max is INT64_MAX to prevent signed
+     * overflows (and to allow for -errno style return values) */
+    assert(!(value >> 63));
+    ((uint64_t *)refcount_array)[index] = cpu_to_be64(value);
+}
+
 
 static int load_refcount_block(BlockDriverState *bs,
                                int64_t refcount_block_offset,