[PULL 0/8] Block layer patches

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:

  Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)

are available in the Git repository at:

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

for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:

  vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)

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

- Fix qmp_device_add() to not throw non-scalar options away (fixes
  iothread-vq-mapping being silently ignored in device_add)
- iotests: Fix mypy failure
- parallels: Avoid potential integer overflow
- Fix crash in migration_is_running()

----------------------------------------------------------------
Dmitry Frolov (1):
      parallels: fix possible int overflow

John Snow (4):
      iotests: reflow ReproducibleTestRunner arguments
      iotests: correct resultclass type in ReproducibleTestRunner
      python: disable too-many-positional-arguments warning
      python: silence pylint raising-non-exception error

Peter Xu (1):
      migration: Check current_migration in migration_is_running()

Stefan Hajnoczi (2):
      qdev-monitor: avoid QemuOpts in QMP device_add
      vl: use qmp_device_add() in qemu_create_cli_devices()

 block/parallels.c             |  4 ++--
 migration/migration.c         |  4 ++++
 system/qdev-monitor.c         | 42 ++++++++++++++++++++++++++++--------------
 system/vl.c                   | 14 ++++----------
 python/scripts/mkvenv.py      |  3 +++
 tests/qemu-iotests/iotests.py | 11 +++++++----
 python/setup.cfg              |  1 +
 tests/qemu-iotests/pylintrc   |  1 +
 8 files changed, 50 insertions(+), 30 deletions(-)

[PULL 1/8] migration: Check current_migration in migration_is_running()

[Kevin Wolf]

From: Peter Xu <peterx@redhat.com>

Report shows that commit 34a8892dec broke iotest 055:

https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org

When replacing migration_is_idle() with "!migration_is_running()", it was
overlooked that the idle helper also checks for current_migration being
available first.

The check would be there if the whole series was applied, but since the
last patches in the previous series rely on some other patches to land
first, we need to recover the behavior of migration_is_idle() first before
that whole set will be merged.

I left migration_is_active / migration_is_device alone, as I don't think
it's possible for them to hit his case (current_migration not initialized).
Also they're prone to removal soon from VFIO side.

Cc: Fabiano Rosas <farosas@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
Reported-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-ID: <20241105182725.2393425-1-peterx@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index aedf7f0751..8c5bd0a75c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1117,6 +1117,10 @@ bool migration_is_running(void)
 {
     MigrationState *s = current_migration;
 
+    if (!s) {
+        return false;
+    }
+
     switch (s->state) {
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-- 
2.47.0

[PULL 2/8] parallels: fix possible int overflow

[Kevin Wolf]

From: Dmitry Frolov <frolov@swemel.ru>

The sum "cluster_index + count" may overflow uint32_t.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
Message-ID: <20241106080521.219255-2-frolov@swemel.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/parallels.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 9205a0864f..071b6dcaf8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -184,11 +184,11 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
     BDRVParallelsState *s = bs->opaque;
     uint32_t cluster_index = host_cluster_index(s, off);
     unsigned long next_used;
-    if (cluster_index + count > bitmap_size) {
+    if ((uint64_t)cluster_index + count > bitmap_size) {
         return -E2BIG;
     }
     next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-    if (next_used < cluster_index + count) {
+    if (next_used < (uint64_t)cluster_index + count) {
         return -EBUSY;
     }
     bitmap_set(bitmap, cluster_index, count);
-- 
2.47.0

[PULL 3/8] iotests: reflow ReproducibleTestRunner arguments

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Trivial reflow to let the type names breathe.

(I need to add a longer type name.)

Signed-off-by: John Snow <jsnow@redhat.com>
Message-ID: <20241101173700.965776-2-jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ea48af4a7b..673bbcd356 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1614,10 +1614,13 @@ def write(self, arg=None):
         self.stream.write(arg)
 
 class ReproducibleTestRunner(unittest.TextTestRunner):
-    def __init__(self, stream: Optional[TextIO] = None,
-                 resultclass: Type[unittest.TestResult] =
-                 ReproducibleTestResult,
-                 **kwargs: Any) -> None:
+    def __init__(
+        self,
+        stream: Optional[TextIO] = None,
+        resultclass: Type[unittest.TestResult] =
+        ReproducibleTestResult,
+        **kwargs: Any
+    ) -> None:
         rstream = ReproducibleStreamWrapper(stream or sys.stdout)
         super().__init__(stream=rstream,           # type: ignore
                          descriptions=True,
-- 
2.47.0

[PULL 4/8] iotests: correct resultclass type in ReproducibleTestRunner

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

I have a vague memory that I suggested this base class to Vladimir and
said "Maybe someday it will break, and I'll just fix it then." Guess
that's today.

Fixes various mypy errors in the "make check-tox" python test for at
least Python3.8; seemingly requires a fairly modern mypy and/or Python
base version to trigger.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-ID: <20241101173700.965776-3-jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 673bbcd356..19817c7353 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1617,7 +1617,7 @@ class ReproducibleTestRunner(unittest.TextTestRunner):
     def __init__(
         self,
         stream: Optional[TextIO] = None,
-        resultclass: Type[unittest.TestResult] =
+        resultclass: Type[unittest.TextTestResult] =
         ReproducibleTestResult,
         **kwargs: Any
     ) -> None:
-- 
2.47.0

[PULL 5/8] python: disable too-many-positional-arguments warning

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Newest versions of pylint complain about specifically positional
arguments in addition to too many in general. We already disable the
general case, so silence this new warning too.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-ID: <20241101173700.965776-4-jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 python/setup.cfg            | 1 +
 tests/qemu-iotests/pylintrc | 1 +
 2 files changed, 2 insertions(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index 3b4e2cc550..cf5af7e664 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -142,6 +142,7 @@ ignore_missing_imports = True
 disable=consider-using-f-string,
         consider-using-with,
         too-many-arguments,
+        too-many-positional-arguments,
         too-many-function-args,  # mypy handles this with less false positives.
         too-many-instance-attributes,
         no-member,  # mypy also handles this better.
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 05b75ee59b..c5f4833e45 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -13,6 +13,7 @@ disable=invalid-name,
         no-else-return,
         too-few-public-methods,
         too-many-arguments,
+        too-many-positional-arguments,
         too-many-branches,
         too-many-lines,
         too-many-locals,
-- 
2.47.0

[PULL 6/8] python: silence pylint raising-non-exception error

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

As of (at least) pylint 3.3.1, this code trips pylint up into believing
we are raising something other than an Exception. We are not: the first
two values may indeed be "None", but the last and final value must by
definition be a SystemExit exception.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-ID: <20241101173700.965776-5-jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 python/scripts/mkvenv.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/python/scripts/mkvenv.py b/python/scripts/mkvenv.py
index f2526af0a0..8ac5b0b2a0 100644
--- a/python/scripts/mkvenv.py
+++ b/python/scripts/mkvenv.py
@@ -379,6 +379,9 @@ def make_venv(  # pylint: disable=too-many-arguments
         try:
             builder.create(str(env_dir))
         except SystemExit as exc:
+            # pylint 3.3 bug:
+            # pylint: disable=raising-non-exception, raise-missing-from
+
             # Some versions of the venv module raise SystemExit; *nasty*!
             # We want the exception that prompted it. It might be a subprocess
             # error that has output we *really* want to see.
-- 
2.47.0

[PULL 7/8] qdev-monitor: avoid QemuOpts in QMP device_add

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The QMP device_add monitor command converts the QDict arguments to
QemuOpts and then back again to QDict. This process only supports scalar
types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
array of objects) are silently dropped by qemu_opts_from_qdict() during
the QemuOpts conversion even though QAPI is capable of validating them.
As a result, hotplugging virtio-blk-pci devices with the
iothread-vq-mapping property does not work as expected (the property is
ignored).

Get rid of the QemuOpts conversion in qmp_device_add() and call
qdev_device_add_from_qdict() with from_json=true. Using the QMP
command's QDict arguments directly allows non-scalar properties.

The HMP is also adjusted since qmp_device_add()'s now expects properly
typed JSON arguments and cannot be used from HMP anymore. Move the code
that was previously in qmp_device_add() (with QemuOpts conversion and
from_json=false) into hmp_device_add() so that its behavior is
unchanged.

This patch changes the behavior of QMP device_add but not HMP
device_add. QMP clients that sent incorrectly typed device_add QMP
commands no longer work. This is a breaking change but clients should be
using the correct types already. See the netdev_add QAPIfication in
commit db2a380c8457 for similar reasoning and object-add in commit
9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
for the time being.

Markus helped me figure this out and even provided a draft patch. The
code ended up very close to what he suggested.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240827192751.948633-2-stefanha@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 system/qdev-monitor.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 4c09b38ffb..03ae610649 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -856,18 +856,9 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
 
 void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
-    QemuOpts *opts;
     DeviceState *dev;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
-    if (!opts) {
-        return;
-    }
-    if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
-        qemu_opts_del(opts);
-        return;
-    }
-    dev = qdev_device_add(opts, errp);
+    dev = qdev_device_add_from_qdict(qdict, true, errp);
     if (!dev) {
         /*
          * Drain all pending RCU callbacks. This is done because
@@ -879,9 +870,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
          * to the user
          */
         drain_call_rcu();
-
-        qemu_opts_del(opts);
-        return;
     }
     object_unref(OBJECT(dev));
 }
@@ -1018,8 +1006,34 @@ void qmp_device_sync_config(const char *id, Error **errp)
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
+    QemuOpts *opts;
+    DeviceState *dev;
 
-    qmp_device_add((QDict *)qdict, NULL, &err);
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &err);
+    if (!opts) {
+        goto out;
+    }
+    if (qdev_device_help(opts)) {
+        qemu_opts_del(opts);
+        return;
+    }
+    dev = qdev_device_add(opts, &err);
+    if (!dev) {
+        /*
+         * Drain all pending RCU callbacks. This is done because
+         * some bus related operations can delay a device removal
+         * (in this case this can happen if device is added and then
+         * removed due to a configuration error)
+         * to a RCU callback, but user might expect that this interface
+         * will finish its job completely once qmp command returns result
+         * to the user
+         */
+        drain_call_rcu();
+
+        qemu_opts_del(opts);
+    }
+    object_unref(dev);
+out:
     hmp_handle_error(mon, err);
 }
 
-- 
2.47.0

[PULL 8/8] vl: use qmp_device_add() in qemu_create_cli_devices()

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

qemu_create_cli_devices() should use qmp_device_add() to match the
behavior of the QMP monitor. A comment explained that libvirt changes
implementing strict CLI syntax were needed.

Peter Krempa <pkrempa@redhat.com> has confirmed that modern libvirt uses
the same JSON for -device (CLI) and device_add (QMP). Go ahead and use
qmp_device_add().

Cc: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240827192751.948633-3-stefanha@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 system/vl.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index d217b3d64d..e3f7d3a156 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2653,17 +2653,11 @@ static void qemu_create_cli_devices(void)
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
-        DeviceState *dev;
+        QObject *ret_data = NULL;
+
         loc_push_restore(&opt->loc);
-        /*
-         * TODO Eventually we should call qmp_device_add() here to make sure it
-         * behaves the same, but QMP still has to accept incorrectly typed
-         * options until libvirt is fixed and we want to be strict on the CLI
-         * from the start, so call qdev_device_add_from_qdict() directly for
-         * now.
-         */
-        dev = qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
-        object_unref(OBJECT(dev));
+        qmp_device_add(opt->opts, &ret_data, &error_fatal);
+        assert(ret_data == NULL); /* error_fatal aborts */
         loc_pop(&opt->loc);
     }
     rom_reset_order_override();
-- 
2.47.0

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
>
>   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
>
>   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Fix qmp_device_add() to not throw non-scalar options away (fixes
>   iothread-vq-mapping being silently ignored in device_add)
> - iotests: Fix mypy failure
> - parallels: Avoid potential integer overflow
> - Fix crash in migration_is_running()
>

Hi; this seems to cause an error for the avocado test
tests/avocado/hotplug_blk.py:HotPlug.test

https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
https://gitlab.com/qemu-project/qemu/-/jobs/8387009383

(12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
(12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
perform graceful shutdown (17.16 s)

If you dig through the build artefacts you can find the debug log:
https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log

and it seems like the test sends a device_add command over
QMP and the result is that QEMU dies with an assertion failure.
The relevant device_add is

[stdlog]   "execute": "device_add",
[stdlog]   "arguments": {
[stdlog]     "driver": "virtio-blk-pci",
[stdlog]     "drive": "disk",
[stdlog]     "id": "virtio-disk0",
[stdlog]     "bus": "pci.1",
[stdlog]     "addr": 1
[stdlog]   },
[stdlog]   "id": "__qmp#00002"
[stdlog] }

Avocado helpfully hides the assertion message under a rock
in a different log file:
https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log

qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
qobject_input_try_get_object: Assertion `removed' failed.


I'm guessing this is Stefan's patches since they touch
the device_add path.

thanks
-- PMM

Re: [PULL 0/8] Block layer patches

[Kevin Wolf]

Am 15.11.2024 um 21:16 hat Peter Maydell geschrieben:
> On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> >
> >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> >
> > are available in the Git repository at:
> >
> >   https://repo.or.cz/qemu/kevin.git tags/for-upstream
> >
> > for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
> >
> >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
> >
> > ----------------------------------------------------------------
> > Block layer patches
> >
> > - Fix qmp_device_add() to not throw non-scalar options away (fixes
> >   iothread-vq-mapping being silently ignored in device_add)
> > - iotests: Fix mypy failure
> > - parallels: Avoid potential integer overflow
> > - Fix crash in migration_is_running()
> >
> 
> Hi; this seems to cause an error for the avocado test
> tests/avocado/hotplug_blk.py:HotPlug.test
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
> https://gitlab.com/qemu-project/qemu/-/jobs/8387009383
> 
> (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
> (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
> perform graceful shutdown (17.16 s)
> 
> If you dig through the build artefacts you can find the debug log:
> https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> 
> and it seems like the test sends a device_add command over
> QMP and the result is that QEMU dies with an assertion failure.
> The relevant device_add is
> 
> [stdlog]   "execute": "device_add",
> [stdlog]   "arguments": {
> [stdlog]     "driver": "virtio-blk-pci",
> [stdlog]     "drive": "disk",
> [stdlog]     "id": "virtio-disk0",
> [stdlog]     "bus": "pci.1",
> [stdlog]     "addr": 1
> [stdlog]   },
> [stdlog]   "id": "__qmp#00002"
> [stdlog] }
> 
> Avocado helpfully hides the assertion message under a rock
> in a different log file:
> https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log
> 
> qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
> qobject_input_try_get_object: Assertion `removed' failed.
> 
> 
> I'm guessing this is Stefan's patches since they touch
> the device_add path.

Yes, this is Stefan's patches exposing a preexisting bug on a new code
path. You can already trigger the same bug on the command line with git
master:

$ ./qemu-system-x86_64 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "addr": 1 }'
qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed.

The problem is that set_pci_devfn() visits the same field twice, which
is not allowed. Apparently the QemuOpts visitor accepts it anyway, but
the QObject one doesn't. I'll write a patch to use the proper alternate
mechanism instead, that should fix it.

Kevin

Re: [PULL 0/8] Block layer patches

[Stefan Hajnoczi]

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

On Tue, Nov 19, 2024 at 12:25:26PM +0100, Kevin Wolf wrote:
> Am 15.11.2024 um 21:16 hat Peter Maydell geschrieben:
> > On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> > >
> > >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   https://repo.or.cz/qemu/kevin.git tags/for-upstream
> > >
> > > for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
> > >
> > >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
> > >
> > > ----------------------------------------------------------------
> > > Block layer patches
> > >
> > > - Fix qmp_device_add() to not throw non-scalar options away (fixes
> > >   iothread-vq-mapping being silently ignored in device_add)
> > > - iotests: Fix mypy failure
> > > - parallels: Avoid potential integer overflow
> > > - Fix crash in migration_is_running()
> > >
> > 
> > Hi; this seems to cause an error for the avocado test
> > tests/avocado/hotplug_blk.py:HotPlug.test
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
> > https://gitlab.com/qemu-project/qemu/-/jobs/8387009383
> > 
> > (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
> > (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
> > perform graceful shutdown (17.16 s)
> > 
> > If you dig through the build artefacts you can find the debug log:
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> > 
> > and it seems like the test sends a device_add command over
> > QMP and the result is that QEMU dies with an assertion failure.
> > The relevant device_add is
> > 
> > [stdlog]   "execute": "device_add",
> > [stdlog]   "arguments": {
> > [stdlog]     "driver": "virtio-blk-pci",
> > [stdlog]     "drive": "disk",
> > [stdlog]     "id": "virtio-disk0",
> > [stdlog]     "bus": "pci.1",
> > [stdlog]     "addr": 1
> > [stdlog]   },
> > [stdlog]   "id": "__qmp#00002"
> > [stdlog] }
> > 
> > Avocado helpfully hides the assertion message under a rock
> > in a different log file:
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log
> > 
> > qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
> > qobject_input_try_get_object: Assertion `removed' failed.
> > 
> > 
> > I'm guessing this is Stefan's patches since they touch
> > the device_add path.
> 
> Yes, this is Stefan's patches exposing a preexisting bug on a new code
> path. You can already trigger the same bug on the command line with git
> master:
> 
> $ ./qemu-system-x86_64 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "addr": 1 }'
> qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed.
> 
> The problem is that set_pci_devfn() visits the same field twice, which
> is not allowed. Apparently the QemuOpts visitor accepts it anyway, but
> the QObject one doesn't. I'll write a patch to use the proper alternate
> mechanism instead, that should fix it.

Thank you, Kevin!

Stefan

> 
> Kevin
> 

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