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(-)
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(-)
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
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
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 :|
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
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 :|
© 2016 - 2024 Red Hat, Inc.