On 30.01.2017 17:52, Eric Blake wrote: > 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?
There's probably not too much that could go wrong, but I wouldn't be
exactly comfortable with it.
>> 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
Kevin once said "We should write code that makes sense today, not code
that will make sense some day in the future." Or something like that. :-)
> (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.
Depends on the viewpoint. You're right, it does prevent us from
accidentally implicitly narrowing a 64 bit size, but on the other hand,
it won't be clear just from looking at the function signature that this
function actually only supports a 32 bit parameter.
Not sure what to do here. Maybe switch to a language that would warn us
before implicitly narrowing values. */me ducks*
As long as the assertion is visible enough (i.e. immediately after the
variable declaration block), it should be fine.
Max
>>> -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.
signature.asc
Description: OpenPGP digital signature
