On Wed, 31 Jul 2024 at 09:59, Peter Krempa <[email protected]> wrote: > > On Tue, Jul 09, 2024 at 16:27:22 +0200, Markus Armbruster wrote: > > Stefan Hajnoczi <[email protected]> writes: > > > > > 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). It's time to QAPIfy QMP device_add! > > > > This patch doesn't fully QAPIfy device_add: we still lack a schema > > and use 'gen': false. It gets us closer, though. > > > > > 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. > > > > Another one is 9151e59a8b6e: it QAPIfied object-add. > > > > Both commits eliminated the roundtrip through QemuOpts, and weaned the > > command off 'gen': false. > > > > This commit eliminates the roundtrip, but keeps 'gen': false. Best we > > can do now, but I'd like the commit message to make this clear. > > > > > 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 <[email protected]> > > > Cc: Daniel P. Berrangé <[email protected]> > > > Signed-off-by: Stefan Hajnoczi <[email protected]> > > > --- > > > system/qdev-monitor.c | 41 ++++++++++++++++++++++++++++------------- > > > 1 file changed, 28 insertions(+), 13 deletions(-) > > [...] > > > Have a look at this TODO in vl.c: > > > > QTAILQ_FOREACH(opt, &device_opts, next) { > > DeviceState *dev; > > 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. > > */ > > So at least this TODO should already be handled on libvirt's side. > > With modern qemu libvirt is using JSON for -device and exactly the same > JSON for device_add. From what I remember from the time when I've > converted -device to use JSON, -device already required the correct > types, which would mean that also libvirt uses the correct types now.
Great, thank you. I will update the code. Stefan
