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

[Max Reitz <mreitz@redhat.com>]

On 2015-02-18 at 21:51, Eric Blake wrote:
> On 02/18/2015 03:40 PM, Max Reitz wrote:
>> Add a refcount_order parameter to qcow2_create2(), use that value for
>> the image header and for calculating the size required for
>> preallocation.
>>
>> For now, always pass 4.
>>
>> This addition requires changes to the calculation of the file size for
>> the "full" and "falloc" preallocation modes. That in turn is a nice
>> opportunity to add a comment about that calculation not necessarily
>> being exact (and that being intentional).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2.c | 47 ++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 36 insertions(+), 11 deletions(-)
>>
>> @@ -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;
>>       int ret;
>>   
>> @@ -2064,8 +2078,19 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>>           goto finish;
>>       }
>>   
>> +    if (version < 3 && refcount_bits != 16) {
>> +        error_setg(errp, "Different refcount widths than 16 bits require "
>> +                   "compatibility level 1.1 or above (use compat=1.1 or "
>> +                   "greater)");
>> +        ret = -EINVAL;
>> +        goto finish;
>> +    }
>> +
>> +    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 <eblake@redhat.com>

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.

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).
>