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 <mre...@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 <ebl...@redhat.com> Thanks! Max
signature.asc
Description: OpenPGP digital signature
