| Message ID | 20211015144640.198044-15-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/15] net: Introduce NetClientInfo.check_peer_type() | expand |
On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> wrote: > QDicts are both what QMP natively uses and what the keyval parser > produces. Going through QemuOpts isn't useful for either one, so switch > the main device creation function to QDicts. By sharing more code with > the -object/object-add code path, we can even reduce the code size a > bit. > > This commit doesn't remove the detour through QemuOpts from any code > path yet, but it allows the following commits to do so. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20211008133442.141332-15-kwolf@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Tested-by: Peter Krempa <pkrempa@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Hi; we discovered via a report on IRC this this commit broke handling of "array properties", of which one example is: qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a This used to work, and now fails with qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property 'rocker.ports[0]' not found I think this happens because array properties have the requirement that the len-foo property is set first before any of the foo[n] properties can be set. In the old code I guess we used to set properties from the command line in the order they were specified, whereas in the new code we end up in object_set_properties_from_qdict() which tries to set them in whatever order the qdict hash table provides them, which turns out to be the wrong one :-( Any suggestions for how to address this ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> wrote: >> QDicts are both what QMP natively uses and what the keyval parser >> produces. Going through QemuOpts isn't useful for either one, so switch >> the main device creation function to QDicts. By sharing more code with >> the -object/object-add code path, we can even reduce the code size a >> bit. >> >> This commit doesn't remove the detour through QemuOpts from any code >> path yet, but it allows the following commits to do so. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> Message-Id: <20211008133442.141332-15-kwolf@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Tested-by: Peter Krempa <pkrempa@redhat.com> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Hi; we discovered via a report on IRC this this commit broke > handling of "array properties", of which one example is: > qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a > > This used to work, and now fails with > qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property > 'rocker.ports[0]' not found > > I think this happens because array properties have the > requirement that the len-foo property is set first before > any of the foo[n] properties can be set. In the old code > I guess we used to set properties from the command line > in the order they were specified, whereas in the new code > we end up in object_set_properties_from_qdict() which > tries to set them in whatever order the qdict hash table > provides them, which turns out to be the wrong one :-( > > Any suggestions for how to address this ? My initial (knee-jerk) reaction to breaking array properties: Faster, Pussycat! Kill! Kill! Back to serious: replace the implementation of QDict so it iterates in order?
Markus Armbruster <armbru@redhat.com> writes: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Fri, 15 Oct 2021 at 16:01, Kevin Wolf <kwolf@redhat.com> wrote: >>> QDicts are both what QMP natively uses and what the keyval parser >>> produces. Going through QemuOpts isn't useful for either one, so switch >>> the main device creation function to QDicts. By sharing more code with >>> the -object/object-add code path, we can even reduce the code size a >>> bit. >>> >>> This commit doesn't remove the detour through QemuOpts from any code >>> path yet, but it allows the following commits to do so. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> Message-Id: <20211008133442.141332-15-kwolf@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Tested-by: Peter Krempa <pkrempa@redhat.com> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> >> Hi; we discovered via a report on IRC this this commit broke >> handling of "array properties", of which one example is: >> qemu-system-x86_64 -netdev user,id=a -device rocker,len-ports=1,ports[0]=a >> >> This used to work, and now fails with >> qemu-system-x86_64: -device rocker,len-ports=1,ports[0]=a: Property >> 'rocker.ports[0]' not found >> >> I think this happens because array properties have the >> requirement that the len-foo property is set first before >> any of the foo[n] properties can be set. In the old code >> I guess we used to set properties from the command line >> in the order they were specified, whereas in the new code >> we end up in object_set_properties_from_qdict() which >> tries to set them in whatever order the qdict hash table >> provides them, which turns out to be the wrong one :-( >> >> Any suggestions for how to address this ? > > My initial (knee-jerk) reaction to breaking array properties: Faster, > Pussycat! Kill! Kill! > > Back to serious: replace the implementation of QDict so it iterates in > order? I just sent [RFC PATCH] qobject: Rewrite implementation of QDict for in-order traversal Message-Id: <20220705095421.2455041-1-armbru@redhat.com> Please test whether this fixes the regressions you observed.
On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: > My initial (knee-jerk) reaction to breaking array properties: Faster, > Pussycat! Kill! Kill! In an ideal world, what would you replace them with? thanks -- PMM
Cc'ing QOM maintainers. Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: >> My initial (knee-jerk) reaction to breaking array properties: Faster, >> Pussycat! Kill! Kill! > > In an ideal world, what would you replace them with? Let's first recapitulate their intended purpose. commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Date: Tue Aug 19 23:55:52 2014 -0700 qom: Add automatic arrayification to object_property_add() If "[*]" is given as the last part of a QOM property name, treat that as an array property. The added property is given the first available name, replacing the * with a decimal number counting from 0. First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so on. Callers may inspect the ObjectProperty * return value to see what number the added property was given. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Signed-off-by: Andreas Färber <afaerber@suse.de> This describes how they work, but sadly not why we want them. For such arcane lore, we need to consult a guru. Possibly via the mailing list archive. Digression: when you add a feature, please, please, *please* explain why you need it right in the commit message. Such rationale is useful information, tends to age well, and can be quite laborious to reconstruct later. Even though I'm sure we discussed the intended purpose(s) of array properties before, a quick grep of my list archive comes up mostly empty, so I'm falling back to (foggy) memory. Please correct mistakes and fill in omissions. We occasionally have a need for an array of properties where the length of the array is not fixed at compile time. Say in code common to several related devices, where some have two frobs, some four, and a future one may have some other number. We could define properties frob0, frob1, ... frobN for some fixed N. Users have to set them like frob0=...,frob1=... and so forth. We need code to reject frobI=... for I exeeding the actual limit. Array properties spare developers picking a fixed N, and users adding an index to the property name. Whether the latter is a good idea is unclear. We need code to reject usage exceeding the actual limit. A secondary use is (was?) avoiding memory region name clashes in code we don't want to touch. Discussed in the review of my attempt to strangle array properties in 2014: Message-ID: <87sihn9nji.fsf@blackfin.pond.sub.org> https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg02103.html
On Fri, Jul 08, 2022 at 01:40:43PM +0200, Markus Armbruster wrote: > Cc'ing QOM maintainers. > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: > >> My initial (knee-jerk) reaction to breaking array properties: Faster, > >> Pussycat! Kill! Kill! > > > > In an ideal world, what would you replace them with? > > Let's first recapitulate their intended purpose. > > commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q > Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Date: Tue Aug 19 23:55:52 2014 -0700 > > qom: Add automatic arrayification to object_property_add() > > If "[*]" is given as the last part of a QOM property name, treat that > as an array property. The added property is given the first available > name, replacing the * with a decimal number counting from 0. > > First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so > on. > > Callers may inspect the ObjectProperty * return value to see what > number the added property was given. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > > This describes how they work, but sadly not why we want them. For such > arcane lore, we need to consult a guru. Possibly via the mailing list > archive. Also doesn't describe why we need to explicitly set the array length upfront, rather than inferring it from the set of elements that are specified, auto-extending the array bounds as we set each property. > Digression: when you add a feature, please, please, *please* explain why > you need it right in the commit message. Such rationale is useful > information, tends to age well, and can be quite laborious to > reconstruct later. > > Even though I'm sure we discussed the intended purpose(s) of array > properties before, a quick grep of my list archive comes up mostly > empty, so I'm falling back to (foggy) memory. Please correct mistakes > and fill in omissions. > > We occasionally have a need for an array of properties where the length > of the array is not fixed at compile time. Say in code common to > several related devices, where some have two frobs, some four, and a > future one may have some other number. > > We could define properties frob0, frob1, ... frobN for some fixed N. > Users have to set them like frob0=...,frob1=... and so forth. We need > code to reject frobI=... for I exeeding the actual limit. > > Array properties spare developers picking a fixed N, and users adding an > index to the property name. Whether the latter is a good idea is > unclear. We need code to reject usage exceeding the actual limit. If we consider that our canonical representation is aiming to be QAPI, and QAPI has unbounded arrays, then by implication if we want a mapping to a flat CLI syntax, then we need some mechanism for unbounded arrays. It would be valid to argue that we shouldn'be be trying to map the full expressiveness of QAPI into a flat CLI syntax though, and should just strive for full JSON everywhere. Indeed every time we have these discussions, I wish we were already at the "full JSON everywhere" point, so we can stop consuming our time debating how to flatten JSON structure into CLI options. But since these array props already exist, we need to find a way out of the problem... With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Jul 08, 2022 at 01:40:43PM +0200, Markus Armbruster wrote: >> Cc'ing QOM maintainers. >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: >> >> My initial (knee-jerk) reaction to breaking array properties: Faster, >> >> Pussycat! Kill! Kill! >> > >> > In an ideal world, what would you replace them with? >> >> Let's first recapitulate their intended purpose. >> >> commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q >> Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Date: Tue Aug 19 23:55:52 2014 -0700 >> >> qom: Add automatic arrayification to object_property_add() >> >> If "[*]" is given as the last part of a QOM property name, treat that >> as an array property. The added property is given the first available >> name, replacing the * with a decimal number counting from 0. >> >> First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so >> on. >> >> Callers may inspect the ObjectProperty * return value to see what >> number the added property was given. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> >> This describes how they work, but sadly not why we want them. For such >> arcane lore, we need to consult a guru. Possibly via the mailing list >> archive. > > Also doesn't describe why we need to explicitly set the array length > upfront, rather than inferring it from the set of elements that are > specified, auto-extending the array bounds as we set each property. > >> Digression: when you add a feature, please, please, *please* explain why >> you need it right in the commit message. Such rationale is useful >> information, tends to age well, and can be quite laborious to >> reconstruct later. >> >> Even though I'm sure we discussed the intended purpose(s) of array >> properties before, a quick grep of my list archive comes up mostly >> empty, so I'm falling back to (foggy) memory. Please correct mistakes >> and fill in omissions. >> >> We occasionally have a need for an array of properties where the length >> of the array is not fixed at compile time. Say in code common to >> several related devices, where some have two frobs, some four, and a >> future one may have some other number. >> >> We could define properties frob0, frob1, ... frobN for some fixed N. >> Users have to set them like frob0=...,frob1=... and so forth. We need >> code to reject frobI=... for I exeeding the actual limit. >> >> Array properties spare developers picking a fixed N, and users adding an >> index to the property name. Whether the latter is a good idea is >> unclear. We need code to reject usage exceeding the actual limit. > > If we consider that our canonical representation is aiming to be QAPI, > and QAPI has unbounded arrays, then by implication if we want a mapping > to a flat CLI syntax, then we need some mechanism for unbounded arrays. > > It would be valid to argue that we shouldn'be be trying to map the full > expressiveness of QAPI into a flat CLI syntax though, and should just > strive for full JSON everywhere. > > Indeed every time we have these discussions, I wish we were already at > the "full JSON everywhere" point, so we can stop consuming our time > debating how to flatten JSON structure into CLI options. But since > these array props already exist, we need to find a way out of the > problem... This isn't just a CLI problem, it's worse: we have property-setting code that relies on "automatic arrayification".
On Fri, 8 Jul 2022 at 12:40, Markus Armbruster <armbru@redhat.com> wrote: > > Cc'ing QOM maintainers. > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: > >> My initial (knee-jerk) reaction to breaking array properties: Faster, > >> Pussycat! Kill! Kill! > > > > In an ideal world, what would you replace them with? > > Let's first recapitulate their intended purpose. > > commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3q > Author: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Date: Tue Aug 19 23:55:52 2014 -0700 > > qom: Add automatic arrayification to object_property_add() > > If "[*]" is given as the last part of a QOM property name, treat that > as an array property. The added property is given the first available > name, replacing the * with a decimal number counting from 0. > > First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so > on. > > Callers may inspect the ObjectProperty * return value to see what > number the added property was given. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > > This describes how they work, but sadly not why we want them. For such > arcane lore, we need to consult a guru. Possibly via the mailing list > archive. This is not the initial array property feature addition. It's adding a convenience extra subfeature (ability to use "[*]" to mean "next one in the array"). The inital array property feature commit is: commit 0be6bfac6262900425c10847de513ee2490078d3 Author: Peter Maydell <peter.maydell@linaro.org> Date: Fri Mar 15 16:41:57 2013 +0000 qdev: Implement (variable length) array properties Add support for declaring array properties for qdev devices. These work by defining an initial static property 'len-arrayname' which the user of the device should set to the desired size of the array. When this property is set, memory is allocated for the array elements, and dynamic properties "arrayname[0]", "arrayname[1]"... are created so the user of the device can then set the values of the individual array elements. Admittedly this doesn't give the justification either :-( The reason is for the benefit of the following commit (and similar use cases): commit 8bd4824a6122292060a318741595927e0d05ff5e Author: Peter Maydell <peter.maydell@linaro.org> Date: Fri Mar 15 16:41:57 2013 +0000 hw/arm_sysctl: Implement SYS_CFG_VOLT Implement the SYS_CFG_VOLT registers which return the voltage of various supplies on motherboard and daughterboard. Since QEMU implements a perfectly stable power supply these registers always return a constant value. The number and value of the daughterboard voltages is dependent on the specific daughterboard, so we use a property array to allow the board to configure them appropriately. > We occasionally have a need for an array of properties where the length > of the array is not fixed at compile time. Say in code common to > several related devices, where some have two frobs, some four, and a > future one may have some other number. > > We could define properties frob0, frob1, ... frobN for some fixed N. > Users have to set them like frob0=...,frob1=... and so forth. We need > code to reject frobI=... for I exeeding the actual limit. > > Array properties spare developers picking a fixed N, and users adding an > index to the property name. Whether the latter is a good idea is > unclear. We need code to reject usage exceeding the actual limit. Note that the initial intention of array properties was largely for within-QEMU-code QOM properties. The usage of array properties for user-creatable devices is a later development. The basic rationale for array properties is: Sometimes the thing we want to be able to configure on a device is naturally expressed as "we have an indexed set of things". When writing a device implementation, it's nice to be able to straightforwardly say "this is an indexed set of things" and have the property system set the device struct fields accordingly (ie setting the num_foo struct field to the number of things and the foo field to an allocated array of foos), and for the way that device users to set the values of that indexed set of things not to look too awful either. The specific implementation we have today is just a cute hack that avoided having to mess about too much with the core QOM property implementation. (And it's only the implementation that imposes the "set the length first" constraint, that's not inherent to the idea of array properties.) I don't have a strong feeling about the "[*]" automatic-indexing subfeature -- none of the use of array properties that I've added uses that. thanks -- PMM
Am 07.07.2022 um 22:24 hat Peter Maydell geschrieben: > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster <armbru@redhat.com> wrote: > > My initial (knee-jerk) reaction to breaking array properties: Faster, > > Pussycat! Kill! Kill! > > In an ideal world, what would you replace them with? The next (and final) patch in this pull request added JSON syntax for -device, so we can actually have proper list properties now that are accessed with the normal list visitors. I suppose some integration with the qdev property system is still missing, but on the QOM level it could be used. In the ideal world, we would also be able to replace the human CLI syntax so that JSON isn't required for this, but doing this in reality would probably cause new compatibility problems - though libvirt has been using JSON for a while, so I guess it wouldn't mind an incompatible change of human syntax. Kevin
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 74d8b614a7..1bad07002d 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -180,7 +180,7 @@ struct DeviceState { char *canonical_path; bool realized; bool pending_deleted_event; - QemuOpts *opts; + QDict *opts; int hotplugged; bool allow_unplug_during_migration; BusState *parent_bus; @@ -205,8 +205,8 @@ struct DeviceListener { * On errors, it returns false and errp is set. Device creation * should fail in this case. */ - bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts, - Error **errp); + bool (*hide_device)(DeviceListener *listener, const QDict *device_opts, + bool from_json, Error **errp); QTAILQ_ENTRY(DeviceListener) link; }; @@ -835,13 +835,15 @@ void device_listener_unregister(DeviceListener *listener); /** * @qdev_should_hide_device: - * @opts: QemuOpts as passed on cmdline. + * @opts: options QDict + * @from_json: true if @opts entries are typed, false for all strings + * @errp: pointer to error object * * Check if a device should be added. * When a device is added via qdev_device_add() this will be called, * and return if the device should be added now or not. */ -bool qdev_should_hide_device(QemuOpts *opts, Error **errp); +bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp); typedef enum MachineInitPhase { /* current_machine is NULL. */ diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h index d118c95f69..74a10ebe85 100644 --- a/include/hw/virtio/virtio-net.h +++ b/include/hw/virtio/virtio-net.h @@ -209,7 +209,8 @@ struct VirtIONet { bool failover_primary_hidden; bool failover; DeviceListener primary_listener; - QemuOpts *primary_opts; + QDict *primary_opts; + bool primary_opts_from_json; Notifier migration_state; VirtioNetRssData rss_data; struct NetRxPkt *rx_pkt; diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h index 74e6c55a2b..1d57bf6577 100644 --- a/include/monitor/qdev.h +++ b/include/monitor/qdev.h @@ -9,6 +9,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp); int qdev_device_help(QemuOpts *opts); DeviceState *qdev_device_add(QemuOpts *opts, Error **errp); +DeviceState *qdev_device_add_from_qdict(const QDict *opts, + bool from_json, Error **errp); /** * qdev_set_id: parent the device and set its id if provided. diff --git a/hw/core/qdev.c b/hw/core/qdev.c index c3a021c444..7f06403752 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -28,6 +28,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qapi/qapi-events-qdev.h" +#include "qapi/qmp/qdict.h" #include "qapi/qmp/qerror.h" #include "qapi/visitor.h" #include "qemu/error-report.h" @@ -211,14 +212,14 @@ void device_listener_unregister(DeviceListener *listener) QTAILQ_REMOVE(&device_listeners, listener, link); } -bool qdev_should_hide_device(QemuOpts *opts, Error **errp) +bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp) { ERRP_GUARD(); DeviceListener *listener; QTAILQ_FOREACH(listener, &device_listeners, link) { if (listener->hide_device) { - if (listener->hide_device(listener, opts, errp)) { + if (listener->hide_device(listener, opts, from_json, errp)) { return true; } else if (*errp) { return false; @@ -958,7 +959,7 @@ static void device_finalize(Object *obj) dev->canonical_path = NULL; } - qemu_opts_del(dev->opts); + qobject_unref(dev->opts); g_free(dev->id); } diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index f503f28c00..09e173a558 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -858,9 +858,11 @@ static void failover_add_primary(VirtIONet *n, Error **errp) return; } - dev = qdev_device_add(n->primary_opts, &err); + dev = qdev_device_add_from_qdict(n->primary_opts, + n->primary_opts_from_json, + &err); if (err) { - qemu_opts_del(n->primary_opts); + qobject_unref(n->primary_opts); n->primary_opts = NULL; } else { object_unref(OBJECT(dev)); @@ -3287,7 +3289,9 @@ static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) } static bool failover_hide_primary_device(DeviceListener *listener, - QemuOpts *device_opts, Error **errp) + const QDict *device_opts, + bool from_json, + Error **errp) { VirtIONet *n = container_of(listener, VirtIONet, primary_listener); const char *standby_id; @@ -3295,7 +3299,7 @@ static bool failover_hide_primary_device(DeviceListener *listener, if (!device_opts) { return false; } - standby_id = qemu_opt_get(device_opts, "failover_pair_id"); + standby_id = qdict_get_try_str(device_opts, "failover_pair_id"); if (g_strcmp0(standby_id, n->netclient_name) != 0) { return false; } @@ -3306,12 +3310,8 @@ static bool failover_hide_primary_device(DeviceListener *listener, return false; } - /* - * Having a weak reference here should be okay because a device can't be - * deleted while it's hidden. This will be replaced soon with a QDict that - * has a clearer ownership model. - */ - n->primary_opts = device_opts; + n->primary_opts = qdict_clone_shallow(device_opts); + n->primary_opts_from_json = from_json; /* failover_primary_hidden is set during feature negotiation */ return qatomic_read(&n->failover_primary_hidden); @@ -3502,8 +3502,11 @@ static void virtio_net_device_unrealize(DeviceState *dev) g_free(n->vlans); if (n->failover) { + qobject_unref(n->primary_opts); device_listener_unregister(&n->primary_listener); remove_migration_state_change_notifier(&n->migration_state); + } else { + assert(n->primary_opts == NULL); } max_queues = n->multiqueue ? n->max_queues : 1; diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 4feaa1cb68..5cdf1d4298 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -29,10 +29,10 @@ #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" #include "migration/vmstate.h" +#include "qapi/qmp/qdict.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" #include "qemu/module.h" -#include "qemu/option.h" #include "qemu/range.h" #include "qemu/units.h" #include "sysemu/kvm.h" @@ -941,7 +941,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) } if (vfio_opt_rom_in_denylist(vdev)) { - if (dev->opts && qemu_opt_get(dev->opts, "rombar")) { + if (dev->opts && qdict_haskey(dev->opts, "rombar")) { warn_report("Device at %s is known to cause system instability" " issues during option rom execution", vdev->vbasedev.name); diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index ea737db028..89c473cb22 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -196,34 +196,6 @@ static void qdev_print_devinfos(bool show_no_user) g_slist_free(list); } -static int set_property(void *opaque, const char *name, const char *value, - Error **errp) -{ - Object *obj = opaque; - QString *val; - Visitor *v; - int ret; - - if (strcmp(name, "driver") == 0) - return 0; - if (strcmp(name, "bus") == 0) - return 0; - - val = qstring_from_str(value); - v = qobject_input_visitor_new_keyval(QOBJECT(val)); - - if (!object_property_set(obj, name, v, errp)) { - ret = -1; - goto out; - } - - ret = 0; -out: - visit_free(v); - qobject_unref(val); - return ret; -} - static const char *find_typename_by_alias(const char *alias) { int i; @@ -624,15 +596,17 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) return prop->name; } -DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) +DeviceState *qdev_device_add_from_qdict(const QDict *opts, + bool from_json, Error **errp) { ERRP_GUARD(); DeviceClass *dc; const char *driver, *path; + char *id; DeviceState *dev = NULL; BusState *bus = NULL; - driver = qemu_opt_get(opts, "driver"); + driver = qdict_get_try_str(opts, "driver"); if (!driver) { error_setg(errp, QERR_MISSING_PARAMETER, "driver"); return NULL; @@ -645,7 +619,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } /* find bus */ - path = qemu_opt_get(opts, "bus"); + path = qdict_get_try_str(opts, "bus"); if (path != NULL) { bus = qbus_find(path, errp); if (!bus) { @@ -665,12 +639,12 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) } } - if (qemu_opt_get(opts, "failover_pair_id")) { - if (!opts->id) { + if (qdict_haskey(opts, "failover_pair_id")) { + if (!qdict_haskey(opts, "id")) { error_setg(errp, "Device with failover_pair_id don't have id"); return NULL; } - if (qdev_should_hide_device(opts, errp)) { + if (qdev_should_hide_device(opts, from_json, errp)) { if (bus && !qbus_is_hotpluggable(bus)) { error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name); } @@ -711,18 +685,24 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) * set dev's parent and register its id. * If it fails it means the id is already taken. */ - if (!qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp)) { + id = g_strdup(qdict_get_try_str(opts, "id")); + if (!qdev_set_id(dev, id, errp)) { goto err_del_dev; } /* set properties */ - if (qemu_opt_foreach(opts, set_property, dev, errp)) { + dev->opts = qdict_clone_shallow(opts); + qdict_del(dev->opts, "driver"); + qdict_del(dev->opts, "bus"); + qdict_del(dev->opts, "id"); + + object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json, + errp); + if (*errp) { goto err_del_dev; } - dev->opts = opts; if (!qdev_realize(DEVICE(dev), bus, errp)) { - dev->opts = NULL; goto err_del_dev; } return dev; @@ -735,6 +715,19 @@ err_del_dev: return NULL; } +/* Takes ownership of @opts on success */ +DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) +{ + QDict *qdict = qemu_opts_to_qdict(opts, NULL); + DeviceState *ret; + + ret = qdev_device_add_from_qdict(qdict, false, errp); + if (ret) { + qemu_opts_del(opts); + } + qobject_unref(qdict); + return ret; +} #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) static void qbus_print(Monitor *mon, BusState *bus, int indent);