Re: [PATCH] qdev-monitor: QAPIfy QMP device_add

[Peter Krempa]

On Tue, Jul 09, 2024 at 16:27:22 +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 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 <armbru@redhat.com>
> > Cc: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  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.