Re: [Qemu-devel] [PATCH v7 11/25] qcow2: refcount_order parameter for qcow2_create2

[Eric Blake]

On 02/19/2015 07:01 AM, Max Reitz wrote:

>>> @@ -2010,6 +2022,8 @@ static int qcow2_create(const char *filename,
>>> QemuOpts *opts, Error **errp)
>>>       size_t cluster_size = DEFAULT_CLUSTER_SIZE;
>>>       PreallocMode prealloc;
>>>       int version = 3;
>>> +    uint64_t refcount_bits = 16;
>>> +    int refcount_order;
>>>       Error *local_err = NULL;

>>> +    refcount_order = ffs(refcount_bits) - 1;
>> ffs() doesn't work on uint64_t (it gives the wrong answer for
>> 0x100000000, for example); you want to use ffsll().  But ffsll() isn't
>> portable.  But we have include/qemu/host-utils.h that gives us ctz64()
>> which is what we want (where ctz==ffs-1 other than for the special case
>> of 0).  So this should be:
>>
>> refcount_order = ctz64(refcount_bits);
>>
>> With that change,
>> Reviewed-by: Eric Blake <address@hidden>
> 
> I intentionally left the ffs() here, because after this patch,
> refcount_bits is guaranteed to be 16, and after patch 14, it's
> guaranteed to be 64 or less (it's more obvious after patch 14 than
> here). So I would be fine with ctz64(), but in my opinion, ffs() is just
> fine, too.

Ah, you are right - here, we are calling ffs(constant), and in patch 14,
the ffs() is quite obviously bounded by a range check, so what you have
works, even if it is not as short as possible, and even though it caused
me to think about integer wrapping.

I'll leave it up to Kevin whether to switch to ctz64() or stick with ffs().

> 
> Max
> 
>> (Hmm, that means that at least qcow2.c:qcow2_create2() has a bug for
>> calling ffs(size_t), and we probably ought to eradicate other uses of
>> ffs from the tree - but as a separate followup).

This still remains true - we should scrub all uses of ffs() on size_t or
uint64_t and make sure they can't suffer from wraparound issues, but
doing that shouldn't hold up this series.

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

signature.asc
Description: OpenPGP digital signature