Re: [PULL v3 0/8] Block layer patches

[Daniel P . Berrangé]

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 :|