[PULL 0/8] Block layer patches

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:

  Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)

are available in the Git repository at:

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

for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:

  iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)

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

- Fix crash on write to read-only devices
- iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
  non-numeric test case names

----------------------------------------------------------------
Kevin Wolf (2):
      block: Separate blk_is_writable() and blk_supports_write_perm()
      virtio-scsi-test: Test writing to scsi-cd device

Vladimir Sementsov-Ogievskiy (6):
      iotests: 146: drop extra whitespaces from .out file
      iotests: add findtests.py
      iotests: add testenv.py
      iotests: add testrunner.py
      iotests: rewrite check into python
      iotests: rename and move 169 and 199 tests

 docs/devel/testing.rst                             |   50 +-
 include/sysemu/block-backend.h                     |    3 +-
 block/block-backend.c                              |   19 +-
 hw/block/dataplane/xen-block.c                     |    2 +-
 hw/block/fdc.c                                     |    9 +-
 hw/block/m25p80.c                                  |    6 +-
 hw/block/nand.c                                    |    2 +-
 hw/block/nvme-ns.c                                 |    7 +-
 hw/block/onenand.c                                 |    2 +-
 hw/block/pflash_cfi01.c                            |    2 +-
 hw/block/pflash_cfi02.c                            |    2 +-
 hw/block/swim.c                                    |    6 +-
 hw/block/virtio-blk.c                              |    6 +-
 hw/block/xen-block.c                               |    2 +-
 hw/ide/core.c                                      |    2 +-
 hw/misc/sifive_u_otp.c                             |    2 +-
 hw/ppc/pnv_pnor.c                                  |    2 +-
 hw/scsi/scsi-disk.c                                |   10 +-
 hw/scsi/scsi-generic.c                             |    4 +-
 hw/sd/sd.c                                         |    6 +-
 hw/usb/dev-storage.c                               |    4 +-
 tests/qtest/virtio-scsi-test.c                     |   39 +
 tests/qemu-iotests/findtests.py                    |  159 +++
 tests/qemu-iotests/iotests.py                      |    8 +
 tests/qemu-iotests/testenv.py                      |  281 +++++
 tests/qemu-iotests/testrunner.py                   |  367 +++++++
 Makefile                                           |    1 -
 tests/check-block.sh                               |    3 +-
 tests/qemu-iotests/146.out                         |  780 +++++++-------
 tests/qemu-iotests/check                           | 1095 +++-----------------
 tests/qemu-iotests/common.env.in                   |    3 -
 tests/qemu-iotests/group                           |  323 ------
 tests/qemu-iotests/meson.build                     |    3 -
 .../{199 => tests/migrate-bitmaps-postcopy-test}   |    0
 .../migrate-bitmaps-postcopy-test.out}             |    0
 .../{169 => tests/migrate-bitmaps-test}            |    0
 .../{169.out => tests/migrate-bitmaps-test.out}    |    0
 37 files changed, 1481 insertions(+), 1729 deletions(-)
 create mode 100644 tests/qemu-iotests/findtests.py
 create mode 100644 tests/qemu-iotests/testenv.py
 create mode 100644 tests/qemu-iotests/testrunner.py
 delete mode 100644 tests/qemu-iotests/common.env.in
 delete mode 100644 tests/qemu-iotests/group
 rename tests/qemu-iotests/{199 => tests/migrate-bitmaps-postcopy-test} (100%)
 rename tests/qemu-iotests/{199.out => tests/migrate-bitmaps-postcopy-test.out} (100%)
 rename tests/qemu-iotests/{169 => tests/migrate-bitmaps-test} (100%)
 rename tests/qemu-iotests/{169.out => tests/migrate-bitmaps-test.out} (100%)

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>
>   Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>
>   iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Fix crash on write to read-only devices
> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>   non-numeric test case names



Applied, thanks.

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

-- PMM

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>
>   Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>
>   iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Fix crash on write to read-only devices
> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>   non-numeric test case names
>
> ----------------------------------------------------------------

I somehow failed to notice before applying this, but this
breaks 'make check' on the netbsd and freebsd VMs.

As usual the build log is pretty opaque, but this looks like the
probable culprit:

env: python3: No such file or directory
gmake: *** [/home/qemu/qemu-test.nU2bcG/src/tests/Makefile.include:144:
check-block] Error 1

The python in the netbsd VM is /usr/pkg/bin/python3.7. Something
seems to be ignoring the --python= passed into our configure
and assuming that "python3" is always a valid executable.

thanks
-- PMM

Re: [PULL 0/8] Block layer patches

[Vladimir Sementsov-Ogievskiy]

28.01.2021 21:19, Peter Maydell wrote:
> On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>>
>>    Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>>
>> are available in the Git repository at:
>>
>>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
>>
>> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>>
>>    iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>>
>> ----------------------------------------------------------------
>> Block layer patches:
>>
>> - Fix crash on write to read-only devices
>> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>>    non-numeric test case names
>>
>> ----------------------------------------------------------------
> 
> I somehow failed to notice before applying this, but this
> breaks 'make check' on the netbsd and freebsd VMs.
> 
> As usual the build log is pretty opaque, but this looks like the
> probable culprit:
> 
> env: python3: No such file or directory
> gmake: *** [/home/qemu/qemu-test.nU2bcG/src/tests/Makefile.include:144:
> check-block] Error 1
> 
> The python in the netbsd VM is /usr/pkg/bin/python3.7. Something
> seems to be ignoring the --python= passed into our configure
> and assuming that "python3" is always a valid executable.
> 

Seems, that's shows that the fact that "PYTHON" variable in check-block.sh works for me doesn't mean that it works everywhere.. My fault, I look closer tomorrow.

-- 
Best regards,
Vladimir

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e:

  Merge tag 'bsd-user-misc-2024q2-pull-request' of gitlab.com:bsdimp/qemu into staging (2024-06-09 11:21:55 -0700)

are available in the Git repository at:

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

for you to fetch changes up to 3ab0f063e58ed9224237d69c4211ca83335164c4:

  crypto/block: drop qcrypto_block_open() n_threads argument (2024-06-10 11:05:43 +0200)

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

- crypto: Fix crash when used with multiqueue devices
- linux-aio: add IO_CMD_FDSYNC command support
- copy-before-write: Avoid integer overflows for timeout > 4s
- Fix crash with QMP block_resize and iothreads
- qemu-io: add cvtnum() error handling for zone commands
- Code cleanup

----------------------------------------------------------------
Denis V. Lunev via (1):
      block: drop force_dup parameter of raw_reconfigure_getfd()

Fiona Ebner (1):
      block/copy-before-write: use uint64_t for timeout in nanoseconds

Prasad J Pandit (1):
      linux-aio: add IO_CMD_FDSYNC command support

Stefan Hajnoczi (5):
      Revert "monitor: use aio_co_reschedule_self()"
      aio: warn about iohandler_ctx special casing
      qemu-io: add cvtnum() error handling for zone commands
      block/crypto: create ciphers on demand
      crypto/block: drop qcrypto_block_open() n_threads argument

 crypto/blockpriv.h             |  13 +++--
 include/block/aio.h            |   6 +++
 include/block/raw-aio.h        |   1 +
 include/crypto/block.h         |   2 -
 block/copy-before-write.c      |   2 +-
 block/crypto.c                 |   1 -
 block/file-posix.c             |  17 ++++--
 block/linux-aio.c              |  21 +++++++-
 block/qcow.c                   |   2 +-
 block/qcow2.c                  |   5 +-
 crypto/block-luks.c            |   4 +-
 crypto/block-qcow.c            |   8 ++-
 crypto/block.c                 | 114 ++++++++++++++++++++++++-----------------
 qapi/qmp-dispatch.c            |   7 ++-
 qemu-io-cmds.c                 |  48 ++++++++++++++++-
 tests/unit/test-crypto-block.c |   4 --
 16 files changed, 176 insertions(+), 79 deletions(-)

Re: [PULL 0/8] Block layer patches

[Richard Henderson]

On 6/11/24 10:36, Kevin Wolf wrote:
> The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e:
> 
>    Merge tag 'bsd-user-misc-2024q2-pull-request' of gitlab.com:bsdimp/qemu into staging (2024-06-09 11:21:55 -0700)
> 
> are available in the Git repository at:
> 
>    https://repo.or.cz/qemu/kevin.git  tags/for-upstream
> 
> for you to fetch changes up to 3ab0f063e58ed9224237d69c4211ca83335164c4:
> 
>    crypto/block: drop qcrypto_block_open() n_threads argument (2024-06-10 11:05:43 +0200)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - crypto: Fix crash when used with multiqueue devices
> - linux-aio: add IO_CMD_FDSYNC command support
> - copy-before-write: Avoid integer overflows for timeout > 4s
> - Fix crash with QMP block_resize and iothreads
> - qemu-io: add cvtnum() error handling for zone commands
> - Code cleanup

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate.


r~

[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 --]