[PATCH] hw: forbid DEFINE_PROP_ARRAY for user creatable devices

[PATCH] hw: forbid DEFINE_PROP_ARRAY for user creatable devices

[Daniel P. Berrangé]

The DEFINE_PROP_ARRAY macro is a clever trick for defining array
properties. It initially creates a property "len-$FOO". When that
property is set, then it creates a sequence "$FOO[NN]" for NN
in the range 0 to "len-$FOO".

The intended usage for this was to simplify code for internal
devices, however, it crept into use for user creatable devices
when the 'rocker' network device used it.

This relied on the user specifying the len property first on
the -device comand line, and the args being processed in-order.
The latter was broken[1] when -device was converted from QemuOpts
to QDict[2], as ordering of loading properties was no longer
guaranteed to match user specified ordering.

This change poisons the setter for "len-$FOO" such that it raises
an error when used with a user creatable device.

This allows DEFINE_PROP_ARRAY to remain exclusively for internal
devices, since code can ensure properties are set in the correct
ordering.

[1] https://gitlab.com/qemu-project/qemu/-/issues/1090
[2] f3558b1b763683bb877f7dd5b282469cdadc65c3
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/qdev-properties.c    | 6 ++++++
 include/hw/qdev-properties.h | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 357b8761b5..2d295411ef 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -584,6 +584,12 @@ static void set_prop_arraylen(Object *obj, Visitor *v, const char *name,
     void *eltptr;
     const char *arrayname;
     int i;
+    DeviceClass *devc = DEVICE_CLASS(object_get_class(obj));
+
+    if (devc->user_creatable) {
+        error_setg(errp, "array property not permitted for user creatable devices");
+        return;
+    }
 
     if (*alenptr) {
         error_setg(errp, "array size property %s may not be set more than once",
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index e1df08876c..19042e6cd9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -127,6 +127,13 @@ extern const PropertyInfo qdev_prop_link;
  * @_arrayprop: PropertyInfo defining what property the array elements have
  * @_arraytype: C type of the array elements
  *
+ * Note: this macro is forbidden to use with user creatable devices
+ *       as its behaviour relies on the precise ordering with which
+ *       properties are set. Ordering is not guaranteed for our public
+ *       facing interfaces (-device CLI / device_add QMP) for creating
+ *       devices. Any attempt to use this on a user creatable device
+ *       will trigger an error at runtime.
+ *
  * Define device properties for a variable-length array _name.  A
  * static property "len-arrayname" is defined. When the device creator
  * sets this property to the desired length of array, further dynamic
-- 
2.41.0

Re: [PATCH] hw: forbid DEFINE_PROP_ARRAY for user creatable devices

[Kevin Wolf]

Am 08.09.2023 um 11:29 hat Daniel P. Berrangé geschrieben:
> The DEFINE_PROP_ARRAY macro is a clever trick for defining array
> properties. It initially creates a property "len-$FOO". When that
> property is set, then it creates a sequence "$FOO[NN]" for NN
> in the range 0 to "len-$FOO".
> 
> The intended usage for this was to simplify code for internal
> devices, however, it crept into use for user creatable devices
> when the 'rocker' network device used it.
> 
> This relied on the user specifying the len property first on
> the -device comand line, and the args being processed in-order.
> The latter was broken[1] when -device was converted from QemuOpts
> to QDict[2], as ordering of loading properties was no longer
> guaranteed to match user specified ordering.
> 
> This change poisons the setter for "len-$FOO" such that it raises
> an error when used with a user creatable device.
> 
> This allows DEFINE_PROP_ARRAY to remain exclusively for internal
> devices, since code can ensure properties are set in the correct
> ordering.
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/1090
> [2] f3558b1b763683bb877f7dd5b282469cdadc65c3
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

NACK.

This doesn't fix the problem, but breaks rocker for good. As I said, I'm
working on proper array support and will send patches soon.

Kevin