Re: [Qemu-devel] [PATCH v4 06/26] qcow2: Use 64 bits for refcount values

[Eric Blake]

On 12/03/2014 06:37 AM, Max Reitz wrote:
> Refcounts may have a width of up to 64 bits, so qemu should use the same
> width to represent refcount values internally.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c | 42 ++++++++++++++++++++----------------------
>  block/qcow2.h          |  4 ++--
>  3 files changed, 23 insertions(+), 25 deletions(-)
> 

> @@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>                                        refcount2 - refcount1,
>                                        refcount1 > refcount2,
>                                        QCOW2_DISCARD_ALWAYS);
> -                if (ret >= 0) {
> +                if (ret == 0) {
>                      (*num_fixed)++;
>                      continue;
>                  }

This hunk looks a bit misplaced (it is unrelated to a sizing change).
Patch 5 altered update_refcount, but does not document its return value.
 But a careful step through update_refcount() shows that all error paths
return negative, and the only places where we do things like 'ret =
alloc_refcount_block(...)" are later followed by an unconditional 'ret =
0' on success paths (so I didn't check whether alloc_refcount_block
returns positive values, but didn't need to).  If you have to respin, it
might be better to add documentation of the return value and update
callers to assume a non-positive return as a separate patch, rather than
sliding it in with this one.  But as I don't see any code faults, and am
not sure it is worth a respin just for this issue,

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature