Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2022-07-27 Thread Kevin Wolf 
Am 07.07.2022 um 22:24 hat Peter Maydell geschrieben:
> On Mon, 4 Jul 2022 at 05:50, Markus Armbruster  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

Re: The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

2022-07-11 Thread Peter Maydell 
On Fri, 8 Jul 2022 at 12:40, Markus Armbruster  wrote:
>
> Cc'ing QOM maintainers.
>
> Peter Maydell  writes:
>
> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster  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 
> 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 
> Signed-off-by: Andreas Färber 
>
> 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 
Date:   Fri Mar 15 16:41:57 2013 +

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 
Date:   Fri Mar 15 16:41:57 2013 +

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

Re: The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

2022-07-08 Thread Daniel P . Berrangé 
On Fri, Jul 08, 2022 at 01:40:43PM +0200, Markus Armbruster wrote:
> Cc'ing QOM maintainers.
> 
> Peter Maydell  writes:
> 
> > On Mon, 4 Jul 2022 at 05:50, Markus Armbruster  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 
> 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 
> Signed-off-by: Andreas Färber 
> 
> 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
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

The case for array properties (was: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts)

2022-07-08 Thread Markus Armbruster 
Cc'ing QOM maintainers.

Peter Maydell  writes:

> On Mon, 4 Jul 2022 at 05:50, Markus Armbruster  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 
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 
Signed-off-by: Andreas Färber 

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@blackfin.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg02103.html

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2022-07-07 Thread Peter Maydell 
On Mon, 4 Jul 2022 at 05:50, Markus Armbruster  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

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2022-07-05 Thread Markus Armbruster 
Markus Armbruster  writes:

> Peter Maydell  writes:
>
>> On Fri, 15 Oct 2021 at 16:01, Kevin Wolf  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 
>>> Message-Id: <20211008133442.141332-15-kw...@redhat.com>
>>> Reviewed-by: Michael S. Tsirkin 
>>> Tested-by: Peter Krempa 
>>> Signed-off-by: Kevin Wolf 
>>
>> 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-arm...@redhat.com>

Please test whether this fixes the regressions you observed.

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2022-07-03 Thread Markus Armbruster 
Peter Maydell  writes:

> On Fri, 15 Oct 2021 at 16:01, Kevin Wolf  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 
>> Message-Id: <20211008133442.141332-15-kw...@redhat.com>
>> Reviewed-by: Michael S. Tsirkin 
>> Tested-by: Peter Krempa 
>> Signed-off-by: Kevin Wolf 
>
> 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?

Re: [PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2022-07-01 Thread Peter Maydell 
On Fri, 15 Oct 2021 at 16:01, Kevin Wolf  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 
> Message-Id: <20211008133442.141332-15-kw...@redhat.com>
> Reviewed-by: Michael S. Tsirkin 
> Tested-by: Peter Krempa 
> Signed-off-by: Kevin Wolf 

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

[PULL 14/15] qdev: Base object creation on QDict rather than QemuOpts

2021-10-15 Thread Kevin Wolf 
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 
Message-Id: <20211008133442.141332-15-kw...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Tested-by: Peter Krempa 
Signed-off-by: Kevin Wolf 
---
 include/hw/qdev-core.h | 12 +++---
 include/hw/virtio/virtio-net.h |  3 +-
 include/monitor/qdev.h |  2 +
 hw/core/qdev.c |  7 ++--
 hw/net/virtio-net.c| 23 +++-
 hw/vfio/pci.c  |  4 +-
 softmmu/qdev-monitor.c | 69 +++---
 7 files changed, 61 insertions(+), 59 deletions(-)

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;
 }
 
-