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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   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 <eblake@redhat.com>

Once again, thank you!

Max