Re: [Qemu-devel] [PATCH v6 04/24] qcow2: Only return status from qcow2_get_refcount

[Kevin Wolf <kwolf@redhat.com>]

Am 10.02.2015 um 21:28 hat Max Reitz geschrieben:
> Refcounts can theoretically be of type uint64_t; in order to be able to
> represent the full range, qcow2_get_refcount() cannot use a single
> variable to represent both all refcount values and also keep some values
> reserved for errors.
> 
> One solution would be to add an Error pointer parameter to
> qcow2_get_refcount(); however, no caller could (currently) pass that
> error message, so it would have to be emitted immediately and be
> passed to the next caller by returning -EIO or something similar.
> Therefore, an Error parameter does not offer any advantages here.
> 
> The solution applied by this patch is simpler to use. Because no caller
> would be able to pass the error message, they would have to print it and
> free it, whereas with this patch the caller only needs to pass the
> returned integer (which is often a no-op from the code perspective,
> because that integer will be stored in a variable "ret" which will be
> returned by the fail path of many callers).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> @@ -1646,13 +1653,14 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int64_t i;
> -    int refcount1, refcount2, ret;
> +    uint16_t refcount1, refcount2;
> +    int ret;
>  
>      for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
> -        refcount1 = qcow2_get_refcount(bs, i);
> -        if (refcount1 < 0) {
> +        ret = qcow2_get_refcount(bs, i, &refcount1);
> +        if (ret < 0) {
>              fprintf(stderr, "Can't get refcount for cluster %" PRId64 ": %s\n",
> -                i, strerror(-refcount1));
> +                    i, strerror(-ret));
>              res->check_errors++;
>              continue;
>          }
> @@ -1682,7 +1690,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>  
>              if (num_fixed) {
>                  ret = update_refcount(bs, i << s->cluster_bits, 1,
> -                                      refcount2 - refcount1,
> +                                      (int)refcount2 - (int)refcount1,
>                                        QCOW2_DISCARD_ALWAYS);
>                  if (ret >= 0) {
>                      (*num_fixed)++;

Wouldn't both refcounts be promoted to int anyway, even without a cast?
But then, being explicit probably can't hurt either.

Kevin