Am 30.08.2024 um 09:29 hat Markus Armbruster geschrieben:
> Stefan Hajnoczi <stefa...@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).
What is the status of this?
This is a bug we certainly want to have fixed for 9.2. It's probably
also something for stable.
> > 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 and object-add in commit
> > 9151e59a8b6e. Unlike those commits, we continue to rely on 'gen': false
> > for the time being.
> >
> > Markus helped me figure this out and even provided a draft patch. The
> > code ended up very close to what he suggested.
>
> Should we discuss the RCU draining issue here?
What RCU draining issue is this?
If I'm reading the patch correctly, it doesn't change anything related
to the RCU logic, but just inlines an existing call for HMP.
> > Suggested-by: Markus Armbruster <arm...@redhat.com>
> > Cc: Daniel P. Berrangé <berra...@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > system/qdev-monitor.c | 44 ++++++++++++++++++++++++++++---------------
> > 1 file changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> > index 6af6ef7d66..26404f314d 100644
> > --- a/system/qdev-monitor.c
> > +++ b/system/qdev-monitor.c
> > @@ -849,18 +849,9 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
> >
> > void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> > {
> > - QemuOpts *opts;
> > DeviceState *dev;
> >
> > - opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
> > - if (!opts) {
> > - return;
> > - }
> > - if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> > - qemu_opts_del(opts);
> > - return;
> > - }
> > - dev = qdev_device_add(opts, errp);
> > + dev = qdev_device_add_from_qdict(qdict, true, errp);
> > if (!dev) {
> > /*
> > * Drain all pending RCU callbacks. This is done because
> > @@ -872,11 +863,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> > Error **errp)
> > * to the user
> > */
> > drain_call_rcu();
> > -
> > - qemu_opts_del(opts);
> > - return;
>
> The removal of return gave me pause. It's actually okay, because the
> code we now execute in addition is a no-op: object_unref(NULL).
>
> Matter of taste.
>
> > }
> > - object_unref(OBJECT(dev));
> > + object_unref(dev);
>
> TIL that object_unref() takes a void *.
>
> commit c5a61e5a3c68144a421117916aef04f2c0fab84b
> Author: Daniel P. Berrangé <berra...@redhat.com>
> Date: Mon Aug 31 17:07:23 2020 -0400
>
> qom: make object_ref/unref use a void * instead of Object *.
>
> The object_ref/unref methods are intended for use with any subclass of
> the base Object. Using "Object *" in the signature is not adding any
> meaningful level of type safety, since callers simply use "OBJECT(ptr)"
> and this expands to an unchecked cast "(Object *)".
>
> By using "void *" we enable the object_unref() method to be used to
> provide support for g_autoptr() with any subclass.
>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> Message-Id: <20200723181410.3145233-2-berra...@redhat.com>
> Message-Id: <20200831210740.126168-2-ehabk...@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
>
> About 2 out of 3 callers still pass an OBJECT(...) argument. Similar
> for object_ref().
>
> If we believe dropping the OBJECT() is an improvement, we should do it
> globally.
>
> Suggest not to touch it in this patch.
Is this the only change you want to see before this is merged?
If so, I can fix it up and take it through my tree.
Kevin
