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