Re: [Qemu-devel] [PATCH v3 07/22] qcow2: Helper function for refcount modification

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

On Thu, Nov 20, 2014 at 06:06:23PM +0100, Max Reitz wrote:
> @@ -116,20 +137,24 @@ int64_t qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index)
>      }
>  
>      ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset,
> -        (void**) &refcount_block);
> +                          &refcount_block);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      block_index = cluster_index & (s->refcount_block_size - 1);
> -    refcount = be16_to_cpu(refcount_block[block_index]);
> +    refcount = s->get_refcount(refcount_block, block_index);
>  
> -    ret = qcow2_cache_put(bs, s->refcount_block_cache,
> -        (void**) &refcount_block);
> +    ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>      if (ret < 0) {
>          return ret;
>      }
>  
> +    if (refcount < 0) {
> +        /* overflow */
> +        return -ERANGE;
> +    }

I guess this is because QEMU uses int64_t but the maximum refcount size
is 64 bits.

If we refuse to open files with 64-bit refcounts, can we drop this
check?

> @@ -583,7 +608,12 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>          /* we can update the count and save it */
>          block_index = cluster_index & (s->refcount_block_size - 1);
>  
> -        refcount = be16_to_cpu(refcount_block[block_index]);
> +        refcount = s->get_refcount(refcount_block, block_index);
> +        if (refcount < 0) {
> +            ret = -ERANGE;
> +            goto fail;
> +        }

Here again.

> @@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs,
>              }
>          }
>  
> -        if (++(*refcount_table)[k] == 0) {
> +        refcount = s->get_refcount(*refcount_table, k);

Here the refcount < 0 check is missing.  That's why it would be simpler
to eliminate the refcount < 0 case entirely.

> @@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>              continue;
>          }
>  
> -        refcount2 = refcount_table[i];
> +        refcount2 = s->get_refcount(refcount_table, i);

Missing here too and in other places.

> +typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array,
> +                                      uint64_t index);

Should the return type be int64_t?

> +typedef void Qcow2SetRefcountFunc(void *refcount_array,
> +                                  uint64_t index, uint64_t value);

Should value's type be int64_t?  Just because the type is unsigned
doesn't make (uint64_t)-1ULL a valid value.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]