Re: [Qemu-devel] [PATCH 1/2] qcow2: Make size_to_clusters() return int64_t

[Max Reitz <mreitz@redhat.com>]

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

On 08.09.2015 22:22, Eric Blake wrote:
> On 09/08/2015 02:09 PM, Max Reitz wrote:
>> Sadly, some images may have more clusters than what can be represented
>> using a plain int. We should be prepared for that case (in
>> qcow2_check_refcounts() we actually were trying to catch that case, but
>> since size_to_clusters() truncated the returned value, that check never
>> did anything useful).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2-cluster.c | 20 +++++++++++---------
>>  block/qcow2.h         |  2 +-
>>  2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 2975b83..a34f0b1 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -473,8 +473,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>>      unsigned int l2_index;
>>      uint64_t l1_index, l2_offset, *l2_table;
>>      int l1_bits, c;
>> -    unsigned int index_in_cluster, nb_clusters;
>> -    uint64_t nb_available, nb_needed;
>> +    unsigned int index_in_cluster;
>> +    uint64_t nb_available, nb_needed, nb_clusters;
> 
> Most uses are storing the results unsigned...
> 
>>  
>> -static inline int size_to_clusters(BDRVQcow2State *s, int64_t size)
>> +static inline int64_t size_to_clusters(BDRVQcow2State *s, int64_t size)
>>  {
>>      return (size + (s->cluster_size - 1)) >> s->cluster_bits;
>>  }
> 
> ...and the function itself doesn't appear to intentionally return
> negative (unless size was passed in as negative, but then that may be
> accidental).  Should it just return uint64_t instead?

It won't matter in practice because we generally don't support any
offsets bigger than INT64_MAX anyway; the @size parameter has been an
int64_t all along, too.

If I have to respin for some reason (i.e. maintainer not willing to fix
up the comment in patch 2), I'll probably change the type, though.

> At any rate, I agree that 'int' is too small, so:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max


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