It's better to pass &error_abort than just assert that result is 0: on
crash, we'll immediately see the reason in the backtrace.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h    |  2 +-
 block/file-posix.c           |  2 +-
 block/io.c                   | 29 ++++++++++++++++++++++-------
 tests/test-write-threshold.c |  5 +++--
 4 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1eeafc118c..ff29f31451 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -93,7 +93,7 @@ typedef struct BdrvTrackedRequest {
     struct BdrvTrackedRequest *waiting_for;
 } BdrvTrackedRequest;
 
-int bdrv_check_request(int64_t offset, int64_t bytes);
+int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
 
 struct BlockDriver {
     const char *format_name;
diff --git a/block/file-posix.c b/block/file-posix.c
index 83e2cc5530..fc35a47832 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2951,7 +2951,7 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
 
         req->bytes = BDRV_MAX_LENGTH - req->offset;
 
-        assert(bdrv_check_request(req->offset, req->bytes) == 0);
+        bdrv_check_request(req->offset, req->bytes, &error_abort);
 
         bdrv_mark_request_serialising(req, bs->bl.request_alignment);
     }
diff --git a/block/io.c b/block/io.c
index 24205f5168..e076236db2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -898,17 +898,34 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
     return waited;
 }
 
-int bdrv_check_request(int64_t offset, int64_t bytes)
+int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
 {
-    if (offset < 0 || bytes < 0) {
+    if (offset < 0) {
+        error_setg(errp, "offset is negative: %" PRIi64, offset);
+        return -EIO;
+    }
+
+    if (bytes < 0) {
+        error_setg(errp, "bytes is negative: %" PRIi64, bytes);
         return -EIO;
     }
 
     if (bytes > BDRV_MAX_LENGTH) {
+        error_setg(errp, "bytes(%" PRIi64 ") exceeds maximum(%" PRIi64 ")",
+                   bytes, BDRV_MAX_LENGTH);
+        return -EIO;
+    }
+
+    if (offset > BDRV_MAX_LENGTH) {
+        error_setg(errp, "offset(%" PRIi64 ") exceeds maximum(%" PRIi64 ")",
+                   offset, BDRV_MAX_LENGTH);
         return -EIO;
     }
 
     if (offset > BDRV_MAX_LENGTH - bytes) {
+        error_setg(errp, "sum of offset(%" PRIi64 ") and bytes(%" PRIi64 ") "
+                   "exceeds maximum(%" PRIi64 ")", offset, bytes,
+                   BDRV_MAX_LENGTH);
         return -EIO;
     }
 
@@ -917,7 +934,7 @@ int bdrv_check_request(int64_t offset, int64_t bytes)
 
 static int bdrv_check_request32(int64_t offset, int64_t bytes)
 {
-    int ret = bdrv_check_request(offset, bytes);
+    int ret = bdrv_check_request(offset, bytes, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -2819,7 +2836,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,
         return -EPERM;
     }
 
-    ret = bdrv_check_request(offset, bytes);
+    ret = bdrv_check_request(offset, bytes, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -3221,10 +3238,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
         return -EINVAL;
     }
 
-    ret = bdrv_check_request(offset, 0);
+    ret = bdrv_check_request(offset, 0, errp);
     if (ret < 0) {
-        error_setg(errp, "Required too big image size, it must be not greater "
-                   "than %" PRId64, BDRV_MAX_LENGTH);
         return ret;
     }
 
diff --git a/tests/test-write-threshold.c b/tests/test-write-threshold.c
index 4cf032652d..fc1c45a2eb 100644
--- a/tests/test-write-threshold.c
+++ b/tests/test-write-threshold.c
@@ -7,6 +7,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "block/block_int.h"
 #include "block/write-threshold.h"
 
@@ -64,7 +65,7 @@ static void test_threshold_not_trigger(void)
     req.offset = 1024;
     req.bytes = 1024;
 
-    assert(bdrv_check_request(req.offset, req.bytes) == 0);
+    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
     amount = bdrv_write_threshold_exceeded(&bs, &req);
@@ -84,7 +85,7 @@ static void test_threshold_trigger(void)
     req.offset = (4 * 1024 * 1024) - 1024;
     req.bytes = 2 * 1024;
 
-    assert(bdrv_check_request(req.offset, req.bytes) == 0);
+    bdrv_check_request(req.offset, req.bytes, &error_abort);
 
     bdrv_write_threshold_set(&bs, threshold);
     amount = bdrv_write_threshold_exceeded(&bs, &req);
-- 
2.25.4

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's better to pass &error_abort than just assert that result is 0: on
> crash, we'll immediately see the reason in the backtrace.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h    |  2 +-
>  block/file-posix.c           |  2 +-
>  block/io.c                   | 29 ++++++++++++++++++++++-------
>  tests/test-write-threshold.c |  5 +++--
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's better to pass &error_abort than just assert that result is 0: on
> crash, we'll immediately see the reason in the backtrace.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h    |  2 +-
>  block/file-posix.c           |  2 +-
>  block/io.c                   | 29 ++++++++++++++++++++++-------
>  tests/test-write-threshold.c |  5 +++--
>  4 files changed, 27 insertions(+), 11 deletions(-)

> +++ b/block/io.c
> @@ -898,17 +898,34 @@ static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self
>      return waited;
>  }
>  
> -int bdrv_check_request(int64_t offset, int64_t bytes)
> +int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp)
>  {

Merge conflicts with 8ac5aab255 here and later in the series, but I
think I managed to resolve all the differences.  I'm planning to queue
this through my NBD tree after subjecting it to more testing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org