On 01/28/2017 02:21 PM, Max Reitz wrote: > On 20.12.2016 20:15, Eric Blake wrote: >> Passing a byte offset, but sector count, when we ultimately >> want to operate on cluster granularity, is madness. Clean up >> the interfaces to take both offset and count as bytes, while >> still keeping the assertion added previously that the caller >> must align the values to a cluster. Then rename things to >> make sure backports don't get confused by changed units: >> instead of qcow2_discard_clusters() and qcow2_zero_clusters(), >> we now have qcow2_cluster_discard and qcow2_cluster_zeroize(). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >>
>> +int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
>> + uint64_t count, int flags);
>
> I know byte count parameters are often called "count", but I find it a
> bit problematic if the function name contains a word that can be a unit.
> In these cases, it's not entirely clear whether "count" refers to bytes
> or clusters. Maybe "length" or "size" would be better?
Now that you mention it, 'cluster' and 'count' is indeed confusing. I'm
leaning towards 'size'. Do I need to respin, or is that something that
the maintainer is comfortable tweaking?
>
> Purely subjective and thus optional, of course.
>
> Another thing: I'm not quite positive whether qcow2_cluster_zeroize()
> can actually handle a byte count greater than INT_MAX. If you could pass
> such a number to it, the multiplication "ret * s->cluster_size" might
> overflow. It's only caller qcow2_co_pwrite_zeroes() will never exceed
> that limit, but maybe this should be made clear somewhere -- either by
> making the parameter an int instead of a uint64_t or by asserting it in
> the function.
I'd lean towards an assertion, especially since it would be nice to
someday unify all the internal block interfaces to get to a 64-bit
interface wherever possible (limiting ourselves to 32-bit is necessary
for some drivers, like NBD, but that limitation should be enforced via
proper BlockLimits, rather than by inherent limits in our API). An
assertion with 64-bit types is better than yet one more place to audit
for missing assertions if we use a 32-bit type now and then widen to
64-bit later.
>> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>> - int nb_sectors, enum qcow2_discard_type type, bool full_discard)
>> +int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
>> + uint64_t count, enum qcow2_discard_type type,
>> + bool full_discard)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> - uint64_t end_offset;
>> + uint64_t end_offset = offset + count;
>> uint64_t nb_clusters;
>> int ret;
>>
>> - end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
>> -
>> /* Caller must pass aligned values */
>> assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>> assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size));
>
> Directly below this we have "offset - end_offset" which could be
> shortened to "count" (or "length" or "size" or...).
Okay, there's now enough comments that it's worth me respinning a v5.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
