[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 11/25] qcow2: refcount_order parameter for qc
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v7 11/25] qcow2: refcount_order parameter for qcow2_create2 |
Date: |
Thu, 19 Feb 2015 16:15:29 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
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
- [Qemu-devel] [PATCH v7 00/25] qcow2: Support refcount orders != 4, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 12/25] qcow2: Use symbolic macros in qcow2_amend_options, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 13/25] iotests: Prepare for refcount_bits option, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 15/25] iotests: Add test for different refcount widths, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 16/25] progress: Allow regressing progress, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 18/25] qcow2: Use error_report() in qcow2_amend_options(), Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 14/25] qcow2: Allow creation with refcount order != 4, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 21/25] qcow2: Use intermediate helper CB for amend, Max Reitz, 2015/02/18
- [Qemu-devel] [PATCH v7 19/25] qcow2: Use abort() instead of assert(false), Max Reitz, 2015/02/18