[PULL v3 0/8] Block layer patches

Kevin Wolf posted 8 patches 3 days, 1 hour ago
Only 0 patches received!
There is a newer version of this series
block/parallels.c                |  4 +--
hw/core/qdev-properties-system.c | 54 ++++++++++++++++++++++++++--------------
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, 82 insertions(+), 48 deletions(-)
[PULL v3 0/8] Block layer patches
Posted by Kevin Wolf 3 days, 1 hour ago
The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:

  Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:

  vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
- Fix qdev property crash with integer PCI addresses and JSON -device
- 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

Kevin Wolf (1):
      qdev: Fix set_pci_devfn() to visit option only once

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 +--
 hw/core/qdev-properties-system.c | 54 ++++++++++++++++++++++++++--------------
 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, 82 insertions(+), 48 deletions(-)
Re: [PULL v3 0/8] Block layer patches
Posted by Peter Maydell 2 days, 23 hours ago
On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
>
>   Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:
>
>   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
> - Fix qdev property crash with integer PCI addresses and JSON -device
> - iotests: Fix mypy failure
> - parallels: Avoid potential integer overflow
> - Fix crash in migration_is_running()

Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:

https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
https://gitlab.com/qemu-project/qemu/-/jobs/8423313162

[stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
: Guest command failed: test -e /sys/block/vda

https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log

Looks like the test called device_add, it succeeded, but
it didn't see the /sys/block/vda node appear in the guest.

(The test logic of "try the command, if it fails sleep for 1
second then try a second time and if that fails call it a
test error" doesn't seem super robust in the face of slow
CI runners, but OTOH it failed the same way on both jobs,
so I don't think that is the culprit here.)

thanks
-- PMM
Re: [PULL v3 0/8] Block layer patches
Posted by Kevin Wolf 1 day, 2 hours ago
Am 20.11.2024 um 14:19 hat Peter Maydell geschrieben:
> On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
> >
> >   Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:
> >
> >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
> > - Fix qdev property crash with integer PCI addresses and JSON -device
> > - iotests: Fix mypy failure
> > - parallels: Avoid potential integer overflow
> > - Fix crash in migration_is_running()
> 
> Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
> https://gitlab.com/qemu-project/qemu/-/jobs/8423313162
> 
> [stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
> 1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
> : Guest command failed: test -e /sys/block/vda
> 
> https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> 
> Looks like the test called device_add, it succeeded, but
> it didn't see the /sys/block/vda node appear in the guest.
> 
> (The test logic of "try the command, if it fails sleep for 1
> second then try a second time and if that fails call it a
> test error" doesn't seem super robust in the face of slow
> CI runners, but OTOH it failed the same way on both jobs,
> so I don't think that is the culprit here.)

This looks like a bug in the test case that was previously cancelled out
by a QEMU bug. :-/

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

What it actually meant is "addr": "1". It's an unfortunate interface,
but string "1" and integer 1 mean different things for PCI address
properties... Going through QemuOpts turned everything into strings, so
that masked the bug in the test case.

Should I just fix the test case and move on, or are we concerned about
other users having a similar bug and want to move the change to 10.0,
keeping device_add with iothread-vq-mapping broken in 9.2?

As far as I can tell, libvirt always uses the string form, so everything
using libvirt should be fine either way. Peter (Krempa), can you
confirm?

Kevin
Re: [PULL v3 0/8] Block layer patches
Posted by Daniel P. Berrangé 1 day, 1 hour ago
On Fri, Nov 22, 2024 at 11:17:39AM +0100, Kevin Wolf wrote:
> Am 20.11.2024 um 14:19 hat Peter Maydell geschrieben:
> > On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
> > >
> > >   Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:
> > >
> > >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
> > > - Fix qdev property crash with integer PCI addresses and JSON -device
> > > - iotests: Fix mypy failure
> > > - parallels: Avoid potential integer overflow
> > > - Fix crash in migration_is_running()
> > 
> > Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313162
> > 
> > [stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
> > 1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
> > : Guest command failed: test -e /sys/block/vda
> > 
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> > 
> > Looks like the test called device_add, it succeeded, but
> > it didn't see the /sys/block/vda node appear in the guest.
> > 
> > (The test logic of "try the command, if it fails sleep for 1
> > second then try a second time and if that fails call it a
> > test error" doesn't seem super robust in the face of slow
> > CI runners, but OTOH it failed the same way on both jobs,
> > so I don't think that is the culprit here.)
> 
> This looks like a bug in the test case that was previously cancelled out
> by a QEMU bug. :-/
> 
> {
>   "execute": "device_add",
>   "arguments": {
>     "driver": "virtio-blk-pci",
>     "drive": "disk",
>     "id": "virtio-disk0",
>     "bus": "pci.1",
>     "addr": 1
>   },
>   "id": "__qmp#00002"
> }
> 
> What it actually meant is "addr": "1". It's an unfortunate interface,
> but string "1" and integer 1 mean different things for PCI address
> properties... Going through QemuOpts turned everything into strings, so
> that masked the bug in the test case.

I never realized that "1" and "1" mean different things in PCI
addresses !

IIUC, "1" decodes as slot 1, function 0, while integer 1  decodes as
slot 0, function 1 .  ie raw integer value is "slot << 3 | function"
IIUC from the code ?

This is both majorly surprising, and rather obscure - did we document
the integer value encoding semantics anywhere ?

AFAICT, they first arrived back in

  commit 768a9ebe188bd0a6172a9a4e64777d21fff7f014
  Author: Paolo Bonzini <pbonzini@redhat.com>
  Date:   Thu Feb 9 09:53:32 2012 +0100

    qdev: accept both strings and integers for PCI addresses
    
    Visitors allow a limited form of polymorphism.  Exploit it to support
    setting the non-legacy PCI address property both as a DD.F string
    and as an 8-bit integer.

    The 8-bit integer form is just too clumsy, it is unlikely that we will
    ever drop it.


I'm guessing that last sentence is a mistake - refering to the new
integer syntax as clumsy & unable to be dropped doesn't make sense.
I assume it was referring to the historical string syntax as the
clumsy part.


> Should I just fix the test case and move on, or are we concerned about
> other users having a similar bug and want to move the change to 10.0,
> keeping device_add with iothread-vq-mapping broken in 9.2?

I think neither syntax really makes sense from a pure QAPI design
POV, as both are inventing a special way to encode 2 distinct
fields within one field which is a QAPI anti-pattern. The "right"
QAPI answer is to express them as 2 distinct fields at the QAPI
level.

Having two distinct ways to express the same concept is redundant.
Perhaps passable if one syntax was actually following QAPI best
practice, even better if one syntax were deprecated.


Is it worth thinking about going through the tedium of deprecating
both of them and replacing the syntax with a preferred QAPI design
pattern ?

If we don't want to replace the existing string format which is
widely used, then is the integer format compelling enough to keep
as an option, given it will easily surprise people who don't read
the non-existant docs about it ? ie deprecate integer syntax and
leave ony the string syntax ?

FWIW, the options for a QAPI design following best practice
design would be

1. "slot" and "function" properties at top level

 {
   "execute": "device_add",
   "arguments": {
     "driver": "virtio-blk-pci",
     "drive": "disk",
     "id": "virtio-disk0",
     "bus": "pci.1",
     "slot": 1,
     "function": 0,
   },
   "id": "__qmp#00002"
 }


2. two element array

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


3. As 1 but with nested struct

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


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PULL v3 0/8] Block layer patches
Posted by Kevin Wolf 1 day ago
Am 22.11.2024 um 12:00 hat Daniel P. Berrangé geschrieben:
> On Fri, Nov 22, 2024 at 11:17:39AM +0100, Kevin Wolf wrote:
> > Am 20.11.2024 um 14:19 hat Peter Maydell geschrieben:
> > > On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kwolf@redhat.com> wrote:
> > > >
> > > > The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
> > > >
> > > >   Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:
> > > >
> > > >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
> > > > - Fix qdev property crash with integer PCI addresses and JSON -device
> > > > - iotests: Fix mypy failure
> > > > - parallels: Avoid potential integer overflow
> > > > - Fix crash in migration_is_running()
> > > 
> > > Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:
> > > 
> > > https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
> > > https://gitlab.com/qemu-project/qemu/-/jobs/8423313162
> > > 
> > > [stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
> > > 1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
> > > : Guest command failed: test -e /sys/block/vda
> > > 
> > > https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> > > 
> > > Looks like the test called device_add, it succeeded, but
> > > it didn't see the /sys/block/vda node appear in the guest.
> > > 
> > > (The test logic of "try the command, if it fails sleep for 1
> > > second then try a second time and if that fails call it a
> > > test error" doesn't seem super robust in the face of slow
> > > CI runners, but OTOH it failed the same way on both jobs,
> > > so I don't think that is the culprit here.)
> > 
> > This looks like a bug in the test case that was previously cancelled out
> > by a QEMU bug. :-/
> > 
> > {
> >   "execute": "device_add",
> >   "arguments": {
> >     "driver": "virtio-blk-pci",
> >     "drive": "disk",
> >     "id": "virtio-disk0",
> >     "bus": "pci.1",
> >     "addr": 1
> >   },
> >   "id": "__qmp#00002"
> > }
> > 
> > What it actually meant is "addr": "1". It's an unfortunate interface,
> > but string "1" and integer 1 mean different things for PCI address
> > properties... Going through QemuOpts turned everything into strings, so
> > that masked the bug in the test case.
> 
> I never realized that "1" and "1" mean different things in PCI
> addresses !
> 
> IIUC, "1" decodes as slot 1, function 0, while integer 1  decodes as
> slot 0, function 1 .  ie raw integer value is "slot << 3 | function"
> IIUC from the code ?
> 
> This is both majorly surprising, and rather obscure - did we document
> the integer value encoding semantics anywhere ?

Did we document anything about most QOM properties anywhere?

But yes, I think your understanding is right. Before Stefan's patch,
when it was interpreted as string "1", it resulted in "addr = 01.0" in
info qtree, now that it's interpreted as integer, we get "addr = 00.1".
And adding merely a function without the device it is part of, it makes
sense that the guest doesn't see it.

> AFAICT, they first arrived back in
> 
>   commit 768a9ebe188bd0a6172a9a4e64777d21fff7f014
>   Author: Paolo Bonzini <pbonzini@redhat.com>
>   Date:   Thu Feb 9 09:53:32 2012 +0100
> 
>     qdev: accept both strings and integers for PCI addresses
>     
>     Visitors allow a limited form of polymorphism.  Exploit it to support
>     setting the non-legacy PCI address property both as a DD.F string
>     and as an 8-bit integer.
> 
>     The 8-bit integer form is just too clumsy, it is unlikely that we will
>     ever drop it.
> 
> 
> I'm guessing that last sentence is a mistake - refering to the new
> integer syntax as clumsy & unable to be dropped doesn't make sense.
> I assume it was referring to the historical string syntax as the
> clumsy part.

It's not completely clear to me what this commit actually intended.

But before it, .set was integer-only, it was only .parse that accepted
strings (and only strings). I'm not familiar enough with qdev history to
tell which was used for which case. I suppose .set was mostly for
internal users and .parse for external interfaces (command line, HMP,
maybe also QMP?)

So if that is right, it added strings to internal users, and integers to
external users. Not sure which part was actually needed and for what
reason.

> > Should I just fix the test case and move on, or are we concerned about
> > other users having a similar bug and want to move the change to 10.0,
> > keeping device_add with iothread-vq-mapping broken in 9.2?
> 
> I think neither syntax really makes sense from a pure QAPI design
> POV, as both are inventing a special way to encode 2 distinct
> fields within one field which is a QAPI anti-pattern. The "right"
> QAPI answer is to express them as 2 distinct fields at the QAPI
> level.

Right. qdev and QAPI still live in two separate worlds. This was never
modelled as a QAPI type/command, device_add doesn't have a schema.

Before the patch in this pull request, you couldn't even have non-scalar
properties, so it wouldn't even have been possible to split it like
that. Unless you make it two totally separate properties, but then you
could set one without the other, which feels like it would only add more
strange cases.

> Having two distinct ways to express the same concept is redundant.
> Perhaps passable if one syntax was actually following QAPI best
> practice, even better if one syntax were deprecated.
> 
> 
> Is it worth thinking about going through the tedium of deprecating
> both of them and replacing the syntax with a preferred QAPI design
> pattern ?

Not sure if it's worth it. But at least it's possible to add the
preferred syntax, by adding a dict branch to the alternate.

> If we don't want to replace the existing string format which is
> widely used, then is the integer format compelling enough to keep
> as an option, given it will easily surprise people who don't read
> the non-existant docs about it ? ie deprecate integer syntax and
> leave ony the string syntax ?

I don't expect many users of our external interfaces to use the integer
syntax, so removing it shouldn't hurt from that point of view.

Internal users in the C code still use the integer version. We would
have to change that first. In a quick grep, I see 14 callers, so that
seems doable.

I also see that the property getter is still always integer. So if you
query the device, that's what you get even in external interfaces. Not
sure if anyone relies on that, though. Probably not.

> FWIW, the options for a QAPI design following best practice
> design would be
> 
> 1. "slot" and "function" properties at top level
> 
>  {
>    "execute": "device_add",
>    "arguments": {
>      "driver": "virtio-blk-pci",
>      "drive": "disk",
>      "id": "virtio-disk0",
>      "bus": "pci.1",
>      "slot": 1,
>      "function": 0,
>    },
>    "id": "__qmp#00002"
>  }
> 
> 
> 2. two element array
> 
>  {
>    "execute": "device_add",
>    "arguments": {
>      "driver": "virtio-blk-pci",
>      "drive": "disk",
>      "id": "virtio-disk0",
>      "bus": "pci.1",
>      "addr": [1, 0]
>    },
>    "id": "__qmp#00002"
>  }
> 
> 
> 3. As 1 but with nested struct
> 
>  {
>    "execute": "device_add",
>    "arguments": {
>      "driver": "virtio-blk-pci",
>      "drive": "disk",
>      "id": "virtio-disk0",
>      "bus": "pci.1",
>      "addr": {
>        "slot": 1,
>        "function": 0
>      }
>    },
>    "id": "__qmp#00002"
>  }

If you ask me, 3. is the only correctly modelled interface. Addresses
are set as a single unit, and they are not just a list of same things,
but the two values have different meanings.

Kevin
Re: [PULL v3 0/8] Block layer patches
Posted by Daniel P. Berrangé 1 day, 2 hours ago
On Fri, Nov 22, 2024 at 11:17:39AM +0100, Kevin Wolf wrote:
> Am 20.11.2024 um 14:19 hat Peter Maydell geschrieben:
> > On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > The following changes since commit e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
> > >
> > >   Merge tag 'pull-target-arm-20241119' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19 14:23:34 +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 83987bf722b6b692bc745b47901be76a1c97140b:
> > >
> > >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20 11:47:49 +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)
> > > - Fix qdev property crash with integer PCI addresses and JSON -device
> > > - iotests: Fix mypy failure
> > > - parallels: Avoid potential integer overflow
> > > - Fix crash in migration_is_running()
> > 
> > Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313162
> > 
> > [stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
> > 1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
> > : Guest command failed: test -e /sys/block/vda
> > 
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> > 
> > Looks like the test called device_add, it succeeded, but
> > it didn't see the /sys/block/vda node appear in the guest.
> > 
> > (The test logic of "try the command, if it fails sleep for 1
> > second then try a second time and if that fails call it a
> > test error" doesn't seem super robust in the face of slow
> > CI runners, but OTOH it failed the same way on both jobs,
> > so I don't think that is the culprit here.)
> 
> This looks like a bug in the test case that was previously cancelled out
> by a QEMU bug. :-/
> 
> {
>   "execute": "device_add",
>   "arguments": {
>     "driver": "virtio-blk-pci",
>     "drive": "disk",
>     "id": "virtio-disk0",
>     "bus": "pci.1",
>     "addr": 1
>   },
>   "id": "__qmp#00002"
> }
> 
> What it actually meant is "addr": "1". It's an unfortunate interface,
> but string "1" and integer 1 mean different things for PCI address
> properties... Going through QemuOpts turned everything into strings, so
> that masked the bug in the test case.
> 
> Should I just fix the test case and move on, or are we concerned about
> other users having a similar bug and want to move the change to 10.0,
> keeping device_add with iothread-vq-mapping broken in 9.2?
> 
> As far as I can tell, libvirt always uses the string form, so everything
> using libvirt should be fine either way. Peter (Krempa), can you
> confirm?

Yes, we're fine.

The code is here:

  https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_command.c#L572

The "s:addr" arg indicates we've reqyested "string" formatting for
the arg, which we've always done, because we need the ability to
pass the "slot:function" pair which can't be represented in int
format.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|