Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add

On 9/24/21 11:04, Kevin Wolf wrote: > Directly call qdev_device_add_from_qdict() for QMP device_add instead of > first going through QemuOpts and converting back to QDict. > > Note that this changes the behaviour of device_add, though in ways that > should be considered bug fixes: > > QemuOpts ignores differences between data types, so you could > successfully pass a string "123" for an integer property, or a string > "on" for a boolean property (and vice versa). After this change, the > correct data type for the property must be used in the JSON input. > > qemu_opts_from_qdict() also silently ignores any options whose value is > a QDict, QList or QNull. > > To illustrate, the following QMP command was accepted before and is now > rejected for both reasons: > > { "execute": "device_add", > "arguments": { "driver": "scsi-cd", > "drive": { "completely": "invalid" }, > "physical_block_size": "4096" } } > > Signed-off-by: Kevin Wolf <kwolf(a)redhat.com&gt; > --- > softmmu/qdev-monitor.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index c09b7430eb..8622ccade6 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict) > qdev_print_devinfos(true); > } > -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > +static void monitor_device_add(QDict *qdict, QObject **ret_data, > + bool from_json, Error **errp) > { > QemuOpts *opts; > DeviceState *dev; > @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp) > qemu_opts_del(opts); > return; > } > - dev = qdev_device_add(opts, errp); > + qemu_opts_del(opts); > + > + dev = qdev_device_add_from_qdict(qdict, from_json, errp); Hi Kevin, I'm wandering if deleting the opts (which remove it from the "device" opts list) is really a no-op ?

The opts list is, eg, traversed in hw/net/virtio-net.c in the function failover_find_primary_device_id() which may be called during the virtio_net_set_features() (a TYPE_VIRTIO_NET method). I do not have the knowledge to tell when this method is called. But If this is after we create the devices. Then the list will be empty at this point now. It seems, there are 2 other calling sites of "qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and net/vhost-vdpa.c