| Message ID | 20230908143703.172758-2-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qdev: Make array properties user accessible again | expand |
On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > Instead of exposing the ugly hack of how we represent arrays in qdev (a > static "foo-len" property and after it is set, dynamically created > "foo[i]" properties) to boards, add an interface that allows setting the > whole array at once. > > Once all internal users of devices with array properties have been > converted to use this function, we can change the implementation to move > away from this hack. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 8/9/23 16:36, Kevin Wolf wrote: > Instead of exposing the ugly hack of how we represent arrays in qdev (a > static "foo-len" property and after it is set, dynamically created > "foo[i]" properties) to boards, add an interface that allows setting the > whole array at once. > > Once all internal users of devices with array properties have been > converted to use this function, we can change the implementation to move > away from this hack. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/hw/qdev-properties.h | 3 +++ > hw/core/qdev-properties.c | 21 +++++++++++++++++++++ > 2 files changed, 24 insertions(+) > +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values) > +{ > + const QListEntry *entry; > + g_autofree char *prop_len = g_strdup_printf("len-%s", name); > + uint32_t i = 0; "unsigned"? Anyway, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > + > + object_property_set_int(OBJECT(dev), prop_len, qlist_size(values), > + &error_abort); > + > + QLIST_FOREACH_ENTRY(values, entry) { > + g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i); > + object_property_set_qobject(OBJECT(dev), prop_idx, entry->value, > + &error_abort); > + i++; > + } > + > + qobject_unref(values); > +}
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 8/9/23 16:36, Kevin Wolf wrote: >> Instead of exposing the ugly hack of how we represent arrays in qdev (a >> static "foo-len" property and after it is set, dynamically created >> "foo[i]" properties) to boards, add an interface that allows setting the >> whole array at once. >> Once all internal users of devices with array properties have been >> converted to use this function, we can change the implementation to move >> away from this hack. >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> include/hw/qdev-properties.h | 3 +++ >> hw/core/qdev-properties.c | 21 +++++++++++++++++++++ >> 2 files changed, 24 insertions(+) > > >> +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values) >> +{ >> + const QListEntry *entry; >> + g_autofree char *prop_len = g_strdup_printf("len-%s", name); >> + uint32_t i = 0; > > "unsigned"? Anyway, Yes, or even plain int. It all gets replaced in the last patch, though. > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> + >> + object_property_set_int(OBJECT(dev), prop_len, qlist_size(values), >> + &error_abort); >> + >> + QLIST_FOREACH_ENTRY(values, entry) { >> + g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i); >> + object_property_set_qobject(OBJECT(dev), prop_idx, entry->value, >> + &error_abort); >> + i++; >> + } >> + >> + qobject_unref(values); >> +}
On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > Instead of exposing the ugly hack of how we represent arrays in qdev (a > static "foo-len" property and after it is set, dynamically created > "foo[i]" properties) to boards, add an interface that allows setting the > whole array at once. > > Once all internal users of devices with array properties have been > converted to use this function, we can change the implementation to move > away from this hack. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/hw/qdev-properties.h | 3 +++ > hw/core/qdev-properties.c | 21 +++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > index e1df08876c..7fa2fdb7c9 100644 > --- a/include/hw/qdev-properties.h > +++ b/include/hw/qdev-properties.h > @@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, > const uint8_t *value); > void qdev_prop_set_enum(DeviceState *dev, const char *name, int value); > > +/* Takes ownership of @values */ > +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values); > + > void *object_field_prop_ptr(Object *obj, Property *prop); Are we happy enough with this interface that I can take this single patch in a series that I'm doing (v2 of https://patchew.org/QEMU/20231017122302.1692902-1-peter.maydell@linaro.org/ "arm/stellaris: convert gamepad input device to qdev"), or should I stick to the old style "set length and element properties by hand" code until this whole series has passed patch review (thus giving you another item to add to the conversion list) ? I went for "include this patch" in v1, but this series has spent longer in code review than I was anticipating at that point. thanks -- PMM
Am 27.10.2023 um 20:06 hat Peter Maydell geschrieben: > On Fri, 8 Sept 2023 at 15:37, Kevin Wolf <kwolf@redhat.com> wrote: > > > > Instead of exposing the ugly hack of how we represent arrays in qdev (a > > static "foo-len" property and after it is set, dynamically created > > "foo[i]" properties) to boards, add an interface that allows setting the > > whole array at once. > > > > Once all internal users of devices with array properties have been > > converted to use this function, we can change the implementation to move > > away from this hack. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > include/hw/qdev-properties.h | 3 +++ > > hw/core/qdev-properties.c | 21 +++++++++++++++++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h > > index e1df08876c..7fa2fdb7c9 100644 > > --- a/include/hw/qdev-properties.h > > +++ b/include/hw/qdev-properties.h > > @@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, > > const uint8_t *value); > > void qdev_prop_set_enum(DeviceState *dev, const char *name, int value); > > > > +/* Takes ownership of @values */ > > +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values); > > + > > void *object_field_prop_ptr(Object *obj, Property *prop); > > Are we happy enough with this interface that I can take this single > patch in a series that I'm doing (v2 of > https://patchew.org/QEMU/20231017122302.1692902-1-peter.maydell@linaro.org/ > "arm/stellaris: convert gamepad input device to qdev"), or should > I stick to the old style "set length and element properties by hand" > code until this whole series has passed patch review (thus giving > you another item to add to the conversion list) ? I went for "include > this patch" in v1, but this series has spent longer in code review > than I was anticipating at that point. As far as I am concerned, feel free to include it in your series. I'm also planning to send v2 of this series soon, I think I'm only missing the build time assertion to check the correct alignment that Markus wants to see. Kevin
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index e1df08876c..7fa2fdb7c9 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -206,6 +206,9 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, const uint8_t *value); void qdev_prop_set_enum(DeviceState *dev, const char *name, int value); +/* Takes ownership of @values */ +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values); + void *object_field_prop_ptr(Object *obj, Property *prop); void qdev_prop_register_global(GlobalProperty *prop); diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 357b8761b5..950ef48e01 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -3,12 +3,14 @@ #include "qapi/error.h" #include "qapi/qapi-types-misc.h" #include "qapi/qmp/qerror.h" +#include "qapi/qmp/qlist.h" #include "qemu/ctype.h" #include "qemu/error-report.h" #include "qapi/visitor.h" #include "qemu/units.h" #include "qemu/cutils.h" #include "qdev-prop-internal.h" +#include "qom/qom-qobject.h" void qdev_prop_set_after_realize(DeviceState *dev, const char *name, Error **errp) @@ -739,6 +741,25 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value) &error_abort); } +void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values) +{ + const QListEntry *entry; + g_autofree char *prop_len = g_strdup_printf("len-%s", name); + uint32_t i = 0; + + object_property_set_int(OBJECT(dev), prop_len, qlist_size(values), + &error_abort); + + QLIST_FOREACH_ENTRY(values, entry) { + g_autofree char *prop_idx = g_strdup_printf("%s[%u]", name, i); + object_property_set_qobject(OBJECT(dev), prop_idx, entry->value, + &error_abort); + i++; + } + + qobject_unref(values); +} + static GPtrArray *global_props(void) { static GPtrArray *gp;
Instead of exposing the ugly hack of how we represent arrays in qdev (a static "foo-len" property and after it is set, dynamically created "foo[i]" properties) to boards, add an interface that allows setting the whole array at once. Once all internal users of devices with array properties have been converted to use this function, we can change the implementation to move away from this hack. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/qdev-properties.h | 3 +++ hw/core/qdev-properties.c | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+)