[PULL 0/9] Block layer patches

[PULL 0/9] Block layer patches

[Kevin Wolf]

The following changes since commit 6f625ce2f21d6a1243065d236298277c56f972d5:

  Merge tag 'pull-request-2024-10-21' of https://gitlab.com/thuth/qemu into staging (2024-10-21 17:12:59 +0100)

are available in the Git repository at:

  https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 04bbc3ee52b32ac465547bb40c1f090a1b8f315a:

  raw-format: Fix error message for invalid offset/size (2024-10-22 17:52:49 +0200)

----------------------------------------------------------------
Block layer patches

- Event throttling for BLOCK_IO_ERROR
- iotests: Fix backup-discard-source test for XFS
- Coverity fixes
- raw-format: Fix error message for invalid offset/size

----------------------------------------------------------------
Kevin Wolf (1):
      raw-format: Fix error message for invalid offset/size

Leonid Kaplan (1):
      block-backend: per-device throttling of BLOCK_IO_ERROR reports

Peter Maydell (4):
      block/gluster: Use g_autofree for string in qemu_gluster_parse_json()
      block/ssh.c: Don't double-check that characters are hex digits
      tests/qemu-iotests/211.out: Update to expect MapEntry 'compressed' field
      block/vdi.c: Make SECTOR_SIZE constant 64-bits

Vladimir Sementsov-Ogievskiy (3):
      iotests/backup-discard-source: convert size variable to be int
      iotests/backup-discard-source: don't use actual-size
      qapi: add qom-path to BLOCK_IO_ERROR event

 qapi/block-core.json                           |  9 ++++--
 block/block-backend.c                          | 21 +++++++++++---
 block/gluster.c                                |  7 ++---
 block/raw-format.c                             |  4 +--
 block/ssh.c                                    | 12 ++++----
 block/vdi.c                                    |  4 +--
 monitor/monitor.c                              |  7 +++--
 tests/qemu-iotests/211.out                     |  6 ++--
 tests/qemu-iotests/tests/backup-discard-source | 39 +++++++++++++++++---------
 9 files changed, 70 insertions(+), 39 deletions(-)

[PULL 1/9] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

In the loop in qemu_gluster_parse_json() we do:

    char *str = NULL;
    for(...) {
        str = g_strdup_printf(...);
        ...
        if (various errors) {
            goto out;
        }
        ...
        g_free(str);
        str = NULL;
    }
    return 0;
out:
    various cleanups;
    g_free(str);
    ...
    return -errno;

Coverity correctly complains that the assignment "str = NULL" at the
end of the loop is unnecessary, because we will either go back to the
top of the loop and overwrite it, or else we will exit the loop and
then exit the function without ever reading str again. The assignment
is there as defensive coding to ensure that str is only non-NULL if
it's a live allocation, so this is intentional.

We can make Coverity happier and simplify the code here by using
g_autofree, since we never need 'str' outside the loop.

Resolves: Coverity CID 1527385
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20241008164708.2966400-2-peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index f03d05251e..e9c038042b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -514,7 +514,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     SocketAddressList **tail;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
-    char *str = NULL;
     const char *ptr;
     int i, type, num_servers;
 
@@ -547,7 +546,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     tail = &gconf->server;
 
     for (i = 0; i < num_servers; i++) {
-        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
+        g_autofree char *str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.",
+                                               i);
         qdict_extract_subqdict(options, &backing_options, str);
 
         /* create opts info from runtime_type_opts list */
@@ -658,8 +658,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         qobject_unref(backing_options);
         backing_options = NULL;
-        g_free(str);
-        str = NULL;
     }
 
     return 0;
@@ -668,7 +666,6 @@ out:
     error_propagate(errp, local_err);
     qapi_free_SocketAddress(gsconf);
     qemu_opts_del(opts);
-    g_free(str);
     qobject_unref(backing_options);
     errno = EINVAL;
     return -errno;
-- 
2.47.0

[PULL 2/9] block/ssh.c: Don't double-check that characters are hex digits

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

In compare_fingerprint() we effectively check whether the characters
in the fingerprint are valid hex digits twice: first we do so with
qemu_isxdigit(), but then the hex2decimal() function also has a code
path where it effectively detects an invalid digit and returns -1.
This causes Coverity to complain because it thinks that we might use
that -1 value in an expression where it would be an integer overflow.

Avoid the double-check of hex digit validity by testing the return
values from hex2decimal() rather than doing separate calls to
qemu_isxdigit().

Since this means we now use the illegal-character return value
from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which
has the same effect since the return type is "unsigned" but
looks less confusing at the callsites when we detect it with
"c0 > 0xf".

Resolves: Coverity CID 1547813
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20241008164708.2966400-3-peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/ssh.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 871e1d4753..9f8140bcb6 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -364,7 +364,7 @@ static unsigned hex2decimal(char ch)
         return 10 + (ch - 'A');
     }
 
-    return -1;
+    return UINT_MAX;
 }
 
 /* Compare the binary fingerprint (hash of host key) with the
@@ -376,13 +376,15 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
     unsigned c;
 
     while (len > 0) {
+        unsigned c0, c1;
         while (*host_key_check == ':')
             host_key_check++;
-        if (!qemu_isxdigit(host_key_check[0]) ||
-            !qemu_isxdigit(host_key_check[1]))
+        c0 = hex2decimal(host_key_check[0]);
+        c1 = hex2decimal(host_key_check[1]);
+        if (c0 > 0xf || c1 > 0xf) {
             return 1;
-        c = hex2decimal(host_key_check[0]) * 16 +
-            hex2decimal(host_key_check[1]);
+        }
+        c = c0 * 16 + c1;
         if (c - *fingerprint != 0)
             return c - *fingerprint;
         fingerprint++;
-- 
2.47.0

[PULL 3/9] tests/qemu-iotests/211.out: Update to expect MapEntry 'compressed' field

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

In commit 52b10c9c0c68e90f in 2023 the QAPI MapEntry struct was
updated to add a 'compressed' field. That commit updated a number
of iotest expected-output files, but missed 211, which is vdi
specific. The result is that
 ./check -vdi
and more specifically
 ./check -vdi 211
fails because the expected and actual output don't match.

Update the reference output.

Cc: qemu-stable@nongnu.org
Fixes: 52b10c9c0c68e90f ("qemu-img: map: report compressed data blocks")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-ID: <20241008164708.2966400-4-peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/211.out | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/211.out b/tests/qemu-iotests/211.out
index f02c75409c..ff9f9a6913 100644
--- a/tests/qemu-iotests/211.out
+++ b/tests/qemu-iotests/211.out
@@ -17,7 +17,7 @@ file format: IMGFMT
 virtual size: 128 MiB (134217728 bytes)
 cluster_size: 1048576
 
-[{"data": false, "depth": 0, "length": 134217728, "present": true, "start": 0, "zero": true}]
+[{"compressed": false, "data": false, "depth": 0, "length": 134217728, "present": true, "start": 0, "zero": true}]
 === Successful image creation (explicit defaults) ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -35,7 +35,7 @@ file format: IMGFMT
 virtual size: 64 MiB (67108864 bytes)
 cluster_size: 1048576
 
-[{"data": false, "depth": 0, "length": 67108864, "present": true, "start": 0, "zero": true}]
+[{"compressed": false, "data": false, "depth": 0, "length": 67108864, "present": true, "start": 0, "zero": true}]
 === Successful image creation (with non-default options) ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -53,7 +53,7 @@ file format: IMGFMT
 virtual size: 32 MiB (33554432 bytes)
 cluster_size: 1048576
 
-[{"data": true, "depth": 0, "length": 3072, "offset": 1024, "present": true, "start": 0, "zero": false}, {"data": true, "depth": 0, "length": 33551360, "offset": 4096, "present": true, "start": 3072, "zero": true}]
+[{"compressed": false, "data": true, "depth": 0, "length": 3072, "offset": 1024, "present": true, "start": 0, "zero": false}, {"compressed": false, "data": true, "depth": 0, "length": 33551360, "offset": 4096, "present": true, "start": 3072, "zero": true}]
 === Invalid BlockdevRef ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vdi", "file": "this doesn't exist", "size": 33554432}}}
-- 
2.47.0

[PULL 4/9] block/vdi.c: Make SECTOR_SIZE constant 64-bits

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

Make the VDI SECTOR_SIZE define be a 64-bit constant; this matches
how we define BDRV_SECTOR_SIZE.  The benefit is that it means that we
don't need to carefully cast to 64-bits when doing operations like
"n_sectors * SECTOR_SIZE" to avoid doing a 32x32->32 multiply, which
might overflow, and which Coverity and other static analysers tend to
warn about.

The specific potential overflow Coverity is highlighting is the one
at the end of vdi_co_pwritev() where we write out n_sectors sectors
to the block map.  This is very unlikely to actually overflow, since
the block map has 4 bytes per block and the maximum number of blocks
in the image must fit into a 32-bit integer.  So this commit is not
fixing a real-world bug.

An inspection of all the places currently using SECTOR_SIZE in the
file shows none which care about the change in its type, except for
one call to error_setg() which needs the format string adjusting.

Resolves: Coverity CID 1508076
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20241008164708.2966400-5-peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vdi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 149e15c831..26f7638f1f 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -87,7 +87,7 @@
 /* Command line option for static images. */
 #define BLOCK_OPT_STATIC "static"
 
-#define SECTOR_SIZE 512
+#define SECTOR_SIZE 512ULL
 #define DEFAULT_CLUSTER_SIZE 1048576
 /* Note: can't use 1 * MiB, because it's passed to stringify() */
 
@@ -442,7 +442,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     } else if (header.sector_size != SECTOR_SIZE) {
         error_setg(errp, "unsupported VDI image (sector size %" PRIu32
-                   " is not %u)", header.sector_size, SECTOR_SIZE);
+                   " is not %llu)", header.sector_size, SECTOR_SIZE);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
-- 
2.47.0

[PULL 5/9] iotests/backup-discard-source: convert size variable to be int

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Make variable reusable in code for checks. Don't care to change "512 *
1024" invocations as they will be dropped in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Message-ID: <20240620144402.65896-2-vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/tests/backup-discard-source | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source
index 2391b12acd..05fbe5d26b 100755
--- a/tests/qemu-iotests/tests/backup-discard-source
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -28,7 +28,7 @@ from iotests import qemu_img_create, qemu_img_map, qemu_io
 temp_img = os.path.join(iotests.test_dir, 'temp')
 source_img = os.path.join(iotests.test_dir, 'source')
 target_img = os.path.join(iotests.test_dir, 'target')
-size = '1M'
+size = 1024 * 1024
 
 
 def get_actual_size(vm, node_name):
@@ -39,9 +39,9 @@ def get_actual_size(vm, node_name):
 
 class TestBackup(iotests.QMPTestCase):
     def setUp(self):
-        qemu_img_create('-f', iotests.imgfmt, source_img, size)
-        qemu_img_create('-f', iotests.imgfmt, temp_img, size)
-        qemu_img_create('-f', iotests.imgfmt, target_img, size)
+        qemu_img_create('-f', iotests.imgfmt, source_img, str(size))
+        qemu_img_create('-f', iotests.imgfmt, temp_img, str(size))
+        qemu_img_create('-f', iotests.imgfmt, target_img, str(size))
         qemu_io('-c', 'write 0 1M', source_img)
 
         self.vm = iotests.VM()
@@ -98,7 +98,7 @@ class TestBackup(iotests.QMPTestCase):
         mapping = qemu_img_map(temp_img)
         self.assertEqual(len(mapping), 1)
         self.assertEqual(mapping[0]['start'], 0)
-        self.assertEqual(mapping[0]['length'], 1024 * 1024)
+        self.assertEqual(mapping[0]['length'], size)
         self.assertEqual(mapping[0]['data'], False)
 
         os.remove(temp_img)
@@ -125,7 +125,7 @@ class TestBackup(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', '')
 
         # Check that data is written to temporary image
-        self.assertGreater(get_actual_size(self.vm, 'temp'), 1024 * 1024)
+        self.assertGreater(get_actual_size(self.vm, 'temp'), size)
 
         self.do_backup()
 
-- 
2.47.0

[PULL 6/9] iotests/backup-discard-source: don't use actual-size

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Relying on disk usage is bad thing, and test just doesn't work on XFS.

Let's instead add a dirty bitmap to track writes to test image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Message-ID: <20240620144402.65896-3-vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 .../qemu-iotests/tests/backup-discard-source  | 29 +++++++++++++------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source
index 05fbe5d26b..17fef9c6d3 100755
--- a/tests/qemu-iotests/tests/backup-discard-source
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -31,12 +31,6 @@ target_img = os.path.join(iotests.test_dir, 'target')
 size = 1024 * 1024
 
 
-def get_actual_size(vm, node_name):
-    nodes = vm.cmd('query-named-block-nodes', flat=True)
-    node = next(n for n in nodes if n['node-name'] == node_name)
-    return node['image']['actual-size']
-
-
 class TestBackup(iotests.QMPTestCase):
     def setUp(self):
         qemu_img_create('-f', iotests.imgfmt, source_img, str(size))
@@ -84,7 +78,12 @@ class TestBackup(iotests.QMPTestCase):
             }
         })
 
-        self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+        self.bitmap = {
+            'node': 'temp',
+            'name': 'bitmap0'
+        }
+
+        self.vm.cmd('block-dirty-bitmap-add', self.bitmap)
 
     def tearDown(self):
         # That should fail, because region is discarded
@@ -113,6 +112,13 @@ class TestBackup(iotests.QMPTestCase):
 
         self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
 
+    def get_bitmap_count(self):
+        nodes = self.vm.cmd('query-named-block-nodes', flat=True)
+        temp = next(n for n in nodes if n['node-name'] == 'temp')
+        bitmap = temp['dirty-bitmaps'][0]
+        assert bitmap['name'] == self.bitmap['name']
+        return bitmap['count']
+
     def test_discard_written(self):
         """
         1. Guest writes
@@ -125,7 +131,7 @@ class TestBackup(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', '')
 
         # Check that data is written to temporary image
-        self.assertGreater(get_actual_size(self.vm, 'temp'), size)
+        self.assertEqual(self.get_bitmap_count(), size)
 
         self.do_backup()
 
@@ -138,13 +144,18 @@ class TestBackup(iotests.QMPTestCase):
         """
         self.do_backup()
 
+        # backup job did discard operation and pollute the bitmap,
+        # we have to clean the bitmap, to check next write
+        self.assertEqual(self.get_bitmap_count(), size)
+        self.vm.cmd('block-dirty-bitmap-clear', self.bitmap)
+
         # Try trigger copy-before-write operation
         result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
         self.assert_qmp(result, 'return', '')
 
         # Check that data is not written to temporary image, as region
         # is discarded from copy-before-write process
-        self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+        self.assertEqual(self.get_bitmap_count(), 0)
 
 
 if __name__ == '__main__':
-- 
2.47.0

[PULL 7/9] qapi: add qom-path to BLOCK_IO_ERROR event

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

We need something more reliable than "device" (which absent in modern
interfaces) and "node-name" (which may absent, and actually don't
specify the device, which is a source of error) to make a per-device
throttling for the event in the following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Message-ID: <20241002151806.592469-2-vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json  |  7 +++++--
 block/block-backend.c | 21 +++++++++++++++++----
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2feae8e697..fe63ba6cbf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5584,6 +5584,8 @@
 #
 # Emitted when a disk I/O error occurs
 #
+# @qom-path: path to the device object in the QOM tree (since 9.2)
+#
 # @device: device name.  This is always present for compatibility
 #     reasons, but it can be empty ("") if the image does not have a
 #     device name associated.
@@ -5614,7 +5616,8 @@
 # .. qmp-example::
 #
 #     <- { "event": "BLOCK_IO_ERROR",
-#          "data": { "device": "ide0-hd1",
+#          "data": { "qom-path": "/machine/unattached/device[0]",
+#                    "device": "ide0-hd1",
 #                    "node-name": "#block212",
 #                    "operation": "write",
 #                    "action": "stop",
@@ -5622,7 +5625,7 @@
 #          "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 ##
 { 'event': 'BLOCK_IO_ERROR',
-  'data': { 'device': 'str', '*node-name': 'str',
+  'data': { 'qom-path': 'str', 'device': 'str', '*node-name': 'str',
             'operation': 'IoOperationType',
             'action': 'BlockErrorAction', '*nospace': 'bool',
             'reason': 'str' } }
diff --git a/block/block-backend.c b/block/block-backend.c
index 7bea43bf72..85bcdedcef 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1019,22 +1019,34 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk)
     return blk->dev;
 }
 
-/* Return the qdev ID, or if no ID is assigned the QOM path, of the block
- * device attached to the BlockBackend. */
-char *blk_get_attached_dev_id(BlockBackend *blk)
+static char *blk_get_attached_dev_id_or_path(BlockBackend *blk, bool want_id)
 {
     DeviceState *dev = blk->dev;
     IO_CODE();
 
     if (!dev) {
         return g_strdup("");
-    } else if (dev->id) {
+    } else if (want_id && dev->id) {
         return g_strdup(dev->id);
     }
 
     return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
 }
 
+/*
+ * Return the qdev ID, or if no ID is assigned the QOM path, of the block
+ * device attached to the BlockBackend.
+ */
+char *blk_get_attached_dev_id(BlockBackend *blk)
+{
+    return blk_get_attached_dev_id_or_path(blk, true);
+}
+
+static char *blk_get_attached_dev_path(BlockBackend *blk)
+{
+    return blk_get_attached_dev_id_or_path(blk, false);
+}
+
 /*
  * Return the BlockBackend which has the device model @dev attached if it
  * exists, else null.
@@ -2125,6 +2137,7 @@ static void send_qmp_error_event(BlockBackend *blk,
 
     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
     qapi_event_send_block_io_error(blk_name(blk),
+                                   blk_get_attached_dev_path(blk),
                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error));
-- 
2.47.0

[PULL 8/9] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Kevin Wolf]

From: Leonid Kaplan <xeor@yandex-team.ru>

BLOCK_IO_ERROR events comes from guest, so we must throttle them.
We still want per-device throttling, so let's use device id as a key.

Signed-off-by: Leonid Kaplan <xeor@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Message-ID: <20241002151806.592469-3-vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 2 ++
 monitor/monitor.c    | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fe63ba6cbf..fd3bcc1c17 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5611,6 +5611,8 @@
 # .. note:: If action is "stop", a STOP event will eventually follow
 #    the BLOCK_IO_ERROR event.
 #
+# .. note:: This event is rate-limited.
+#
 # Since: 0.13
 #
 # .. qmp-example::
diff --git a/monitor/monitor.c b/monitor/monitor.c
index db52a9c7ef..56786c0ccc 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -308,6 +308,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
+    [QAPI_EVENT_BLOCK_IO_ERROR]    = { 1000 * SCALE_MS },
     [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
     [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
     [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
@@ -493,7 +494,8 @@ static unsigned int qapi_event_throttle_hash(const void *key)
         hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
     }
 
-    if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
+    if (evstate->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE ||
+        evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
         hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
     }
 
@@ -519,7 +521,8 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
                        qdict_get_str(evb->data, "node-name"));
     }
 
-    if (eva->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE) {
+    if (eva->event == QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE ||
+        eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
         return !strcmp(qdict_get_str(eva->data, "qom-path"),
                        qdict_get_str(evb->data, "qom-path"));
     }
-- 
2.47.0

[PULL 9/9] raw-format: Fix error message for invalid offset/size

[Kevin Wolf]

s->offset and s->size are only set at the end of the function and still
contain the old values when formatting the error message. Print the
parameters with the new values that we actually checked instead.

Fixes: 500e2434207d ('raw-format: Split raw_read_options()')
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240829185527.47152-1-kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-format.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index ac7e8495f6..e08526e2ec 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -111,7 +111,7 @@ raw_apply_options(BlockDriverState *bs, BDRVRawState *s, uint64_t offset,
     if (offset > real_size) {
         error_setg(errp, "Offset (%" PRIu64 ") cannot be greater than "
                    "size of the containing file (%" PRId64 ")",
-                   s->offset, real_size);
+                   offset, real_size);
         return -EINVAL;
     }
 
@@ -119,7 +119,7 @@ raw_apply_options(BlockDriverState *bs, BDRVRawState *s, uint64_t offset,
         error_setg(errp, "The sum of offset (%" PRIu64 ") and size "
                    "(%" PRIu64 ") has to be smaller or equal to the "
                    " actual size of the containing file (%" PRId64 ")",
-                   s->offset, s->size, real_size);
+                   offset, size, real_size);
         return -EINVAL;
     }
 
-- 
2.47.0

Re: [PULL 0/9] Block layer patches

[Peter Maydell]

On Tue, 22 Oct 2024 at 17:51, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 6f625ce2f21d6a1243065d236298277c56f972d5:
>
>   Merge tag 'pull-request-2024-10-21' of https://gitlab.com/thuth/qemu into staging (2024-10-21 17:12:59 +0100)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 04bbc3ee52b32ac465547bb40c1f090a1b8f315a:
>
>   raw-format: Fix error message for invalid offset/size (2024-10-22 17:52:49 +0200)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Event throttling for BLOCK_IO_ERROR
> - iotests: Fix backup-discard-source test for XFS
> - Coverity fixes
> - raw-format: Fix error message for invalid offset/size
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.2
for any user-visible changes.

-- PMM