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

From: Olaf Hering <olaf@aepfle.de>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-block@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	"open list:All patches CC here" <qemu-devel@nongnu.org>,
	Max Reitz <mreitz@redhat.com>,
	"open list:X86" <xen-devel@lists.xensource.com>,
	Anthony Perard <anthony.perard@citrix.com>
Subject: Re: [Qemu-devel] [PATCH] xen_disk: convert discard input to byte ranges
Date: Fri, 18 Nov 2016 18:41:09 +0100	[thread overview]
Message-ID: <20161118174108.GF5717@aepfle.de> (raw)
In-Reply-To: <42ca6186-ca47-3c63-d2e0-54f2ed9f4be7@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

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)) {

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

Olaf

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 163 bytes --]