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

[Max Reitz]

On 2014-12-03 at 17:11, Eric Blake wrote:
> 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).

Ah, right, I think it should have been in patch 3. I even looked through 
the patches before sending them, but somehow I missed this...

> 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,

Hm, I don't think this is something that should be fixed up by the 
applying maintainer. I will respin, even if it's just for this, but I'll 
wait until this series has been sufficiently looked at.

> Reviewed-by: Eric Blake <address@hidden>

Once again, thank you!

Max