|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters |
Date: | Wed, 22 Apr 2020 20:45:50 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
22.04.2020 18:50, Stefan Hajnoczi wrote:
On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:@@ -875,9 +875,9 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self }static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,- size_t size) + int64_t bytes) { - if (size > BDRV_REQUEST_MAX_BYTES) { + if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) { return -EIO; }@@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,return -ENOMEDIUM; }- if (offset < 0) {- return -EIO; - } - return 0; }Moving this if statement was unnecessary. I'm not sure if there are cases where callers will now see EIO instead of ENOMEDIUM and change their behavior :(. More conservative code changes are easier to review and merge because they are less risky.
Hmm, would be a bit strange to just add "bytes < 0" to the first "if" keeping "offset < 0" in the third. And strange to keep "bytes > BDRV_REQUEST_MAX_BYTES" in the first, if add "bytes < 0" to the third..
@@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child, }static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,- int64_t offset, int bytes, BdrvRequestFlags flags) + int64_t offset, int64_t bytes, BdrvRequestFlags flags) { BlockDriver *drv = bs->drv; QEMUIOVector qiov; @@ -1773,7 +1770,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, assert(max_write_zeroes >= bs->bl.request_alignment);while (bytes > 0 && !ret) {- int num = bytes; + int64_t num = bytes;/* Align request. Block drivers can expect the "bulk" of the request* to be aligned, and that unaligned requests do not cross cluster @@ -1799,6 +1796,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, ret = -ENOTSUP; /* First try the efficient write zeroes operation */ if (drv->bdrv_co_pwrite_zeroes) { + assert(num <= INT_MAX);max_write_zeroes already enforces this, so the assertion is always false: int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX); ... /* limit request size */ if (num > max_write_zeroes) { num = max_write_zeroes; }
You are right, I'll drop it. -- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |