Re: [Qemu-devel] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte count

[Eric Blake]

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 <address@hidden>
>>

>> +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