Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges

[Eric Blake]

On 11/18/2016 11:41 AM, Olaf Hering wrote:
> On Fri, Nov 18, Eric Blake wrote:
> 
>> On 11/18/2016 04:24 AM, Olaf Hering wrote:
>>> +    /* Overflowing byte limit? */
>>> +    if ((sec_start + sec_count) > ((INT64_MAX + INT_MAX) >> 
>>> BDRV_SECTOR_BITS)) {
>> This is undefined.  INT64_MAX + anything non-negative overflows int64,
> 
> The expanded value used to be stored into a uint64_t before it was used
> here. A "cleanup" introduced this error. Thanks for spotting.
> 
>> If you are trying to detect guests that make a request that would cover
>> more than INT64_MAX bytes, you can simplify.  Besides, for as much
>> storage as there is out there, I seriously doubt ANYONE will ever have
>> 2^63 bytes addressable through a single device.  Why not just write it as:
>>
>> if ((INT64_MAX >> BDRV_SECTOR_BITS) - sec_count < sec_start) {
> 
> That would always be false I think. I will resubmit with this:
> if ((sec_start + sec_count) > (INT64_MAX >> BDRV_SECTOR_BITS)) {

You're testing whether something overflows, but you don't want to cause
overflow as part of the test.  So use the commutative law to rewrite it
to avoid sec_start+sec_count from overflowing, and you get:

if (sec_start > (INT64_MAX >> BDRV_SECTOR_BITS) - sec_count)

but that's exactly the expression I wrote above.

> 
> Regarding the cast for ->req, it has type blkif_request_t, but the
> pointer needs to be assigned to type blkif_request_discard_t.

Then why is the cast to (void*) instead of (blkif_request_discard_t*) ?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature