| Message ID | 20241021163538.136941-11-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: miscellaneous cleanups + QOM integration tests | expand |
Hi, On 21/10/24 13:35, Paolo Bonzini wrote: > Constify all accesses to qdev properties, except for the > ObjectPropertyAccessor itself. This makes it possible to place them in > read-only memory, and also lets Rust bindings switch from "static mut" > arrays to "static"; which is advantageous, because mutable statics are > highly discouraged. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > include/hw/qdev-core.h | 4 ++-- > include/hw/qdev-properties.h | 4 ++-- > hw/core/qdev-properties.c | 26 +++++++++++++------------- > system/qdev-monitor.c | 2 +- > 4 files changed, 18 insertions(+), 18 deletions(-) > -void qdev_property_add_static(DeviceState *dev, Property *prop) > +void qdev_property_add_static(DeviceState *dev, const Property *prop) > { > Object *obj = OBJECT(dev); > ObjectProperty *op; > @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) > field_prop_getter(prop->info), > field_prop_setter(prop->info), > prop->info->release, > - prop); > + (Property *)prop); I like the overall patch idea, but I'm not keep on casting const to non-const. Should we adapt the callee -- here object_property_add() -- to also take a const argument? > > object_property_set_description(obj, prop->name, > prop->info->description); > @@ -994,7 +994,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) > } > > static void qdev_class_add_property(DeviceClass *klass, const char *name, > - Property *prop) > + const Property *prop) > { > ObjectClass *oc = OBJECT_CLASS(klass); > ObjectProperty *op; > @@ -1007,7 +1007,7 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name, > field_prop_getter(prop->info), > field_prop_setter(prop->info), > prop->info->release, > - prop); > + (Property *)prop); > } > if (prop->set_default) { > prop->info->set_default_value(op, prop); > @@ -1046,7 +1046,7 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v, > * Do not use this in new code! QOM Properties added through this interface > * will be given names in the "legacy" namespace. > */ > -static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop) > +static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop) > { > g_autofree char *name = NULL; > > @@ -1058,12 +1058,12 @@ static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop) > name = g_strdup_printf("legacy-%s", prop->name); > object_class_property_add(OBJECT_CLASS(dc), name, "str", > prop->info->print ? qdev_get_legacy_property : prop->info->get, > - NULL, NULL, prop); > + NULL, NULL, (Property *)prop); > }
On Tue, Oct 22, 2024 at 6:31 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > -void qdev_property_add_static(DeviceState *dev, Property *prop) > > +void qdev_property_add_static(DeviceState *dev, const Property *prop) > > { > > Object *obj = OBJECT(dev); > > ObjectProperty *op; > > @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) > > field_prop_getter(prop->info), > > field_prop_setter(prop->info), > > prop->info->release, > > - prop); > > + (Property *)prop); > > I like the overall patch idea, but I'm not keen on casting > const to non-const. Should we adapt the callee -- here > object_property_add() -- to also take a const argument? This argument goes into prop->opaque and is passed to all accessor/resolver/finalizers functions. So it would be a much larger change because it needs to change all those functions from "void *opaque" to "const void *opaque". It would also be an issue because some finalizers write to the opaque for good reason: static void object_finalize_child_property( Object *obj, const char *name, void *opaque) { Object *child = opaque; if (child->class->unparent) { (child->class->unparent)(child); } child->parent = NULL; // <--- here object_unref(child); } So, it's not great but it seems necessary. At least keeping the const within qdev properties makes things "safer" within that realm. Paolo
On 22/10/24 02:23, Paolo Bonzini wrote: > On Tue, Oct 22, 2024 at 6:31 AM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: >>> -void qdev_property_add_static(DeviceState *dev, Property *prop) >>> +void qdev_property_add_static(DeviceState *dev, const Property *prop) >>> { >>> Object *obj = OBJECT(dev); >>> ObjectProperty *op; >>> @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) >>> field_prop_getter(prop->info), >>> field_prop_setter(prop->info), >>> prop->info->release, >>> - prop); >>> + (Property *)prop); >> >> I like the overall patch idea, but I'm not keen on casting >> const to non-const. Should we adapt the callee -- here >> object_property_add() -- to also take a const argument? > > This argument goes into prop->opaque and is passed to all > accessor/resolver/finalizers functions. So it would be a much larger > change because it needs to change all those functions from "void > *opaque" to "const void *opaque". > > It would also be an issue because some finalizers write to the opaque > for good reason: > > static void object_finalize_child_property( > Object *obj, const char *name, void *opaque) > { > Object *child = opaque; > > if (child->class->unparent) { > (child->class->unparent)(child); > } > child->parent = NULL; // <--- here > object_unref(child); > } > > So, it's not great but it seems necessary. At least keeping the const > within qdev properties makes things "safer" within that realm. Since it is only within qdev-properties.c, it is indeed reasonable to accept. Maybe make it explicit via a well-named macro to do the cast? /* NON_CONST_PROP: Specific macro to this file because ... */ #define NON_CONST_PROP(prop) (Property *)(prop) Regards, Phil.
Il mar 22 ott 2024, 23:43 Philippe Mathieu-Daudé <philmd@linaro.org> ha scritto: > On 22/10/24 02:23, Paolo Bonzini wrote: > > At least keeping the const > > within qdev properties makes things "safer" within that realm. > > Since it is only within qdev-properties.c, it is indeed reasonable to > accept. Maybe make it explicit via a well-named macro to do the cast? > > /* NON_CONST_PROP: Specific macro to this file because ... */ > #define NON_CONST_PROP(prop) (Property *)(prop) > The name of the macro suggests the opposite, i.e. that you'd use it to cast const void* to Property* in the callbacks. I would leave it as it is. Paolo > > Regards, > > Phil. > >
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index aa97c34a4be..f9fa291cc63 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -132,7 +132,7 @@ struct DeviceClass { * ensures a compile-time error if someone attempts to assign * dc->props directly. */ - Property *props_; + const Property *props_; /** * @user_creatable: Can user instantiate with -device / device_add? @@ -935,7 +935,7 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); * you attempt to add an existing property defined by a parent class. * To modify an inherited property you need to use???? */ -void device_class_set_props(DeviceClass *dc, Property *props); +void device_class_set_props(DeviceClass *dc, const Property *props); /** * device_class_set_parent_realize() - set up for chaining realize fns diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 09aa04ca1e2..26ebd230685 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -37,7 +37,7 @@ struct PropertyInfo { int (*print)(Object *obj, Property *prop, char *dest, size_t len); void (*set_default_value)(ObjectProperty *op, const Property *prop); ObjectProperty *(*create)(ObjectClass *oc, const char *name, - Property *prop); + const Property *prop); ObjectPropertyAccessor *get; ObjectPropertyAccessor *set; ObjectPropertyRelease *release; @@ -223,7 +223,7 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, Object *obj, * On error, store error in @errp. Static properties access data in a struct. * The type of the QOM property is derived from prop->info. */ -void qdev_property_add_static(DeviceState *dev, Property *prop); +void qdev_property_add_static(DeviceState *dev, const Property *prop); /** * qdev_alias_all_properties: Create aliases on source for all target properties diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 86a583574dd..315196bd85a 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -749,7 +749,7 @@ const PropertyInfo qdev_prop_array = { /* --- public helpers --- */ -static Property *qdev_prop_walk(Property *props, const char *name) +static const Property *qdev_prop_walk(const Property *props, const char *name) { if (!props) { return NULL; @@ -763,10 +763,10 @@ static Property *qdev_prop_walk(Property *props, const char *name) return NULL; } -static Property *qdev_prop_find(DeviceState *dev, const char *name) +static const Property *qdev_prop_find(DeviceState *dev, const char *name) { ObjectClass *class; - Property *prop; + const Property *prop; /* device properties */ class = object_get_class(OBJECT(dev)); @@ -840,7 +840,7 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value) void qdev_prop_set_enum(DeviceState *dev, const char *name, int value) { - Property *prop; + const Property *prop; prop = qdev_prop_find(dev, name); object_property_set_str(OBJECT(dev), name, @@ -956,7 +956,7 @@ const PropertyInfo qdev_prop_size = { /* --- object link property --- */ static ObjectProperty *create_link_property(ObjectClass *oc, const char *name, - Property *prop) + const Property *prop) { return object_class_property_add_link(oc, name, prop->link_type, prop->offset, @@ -969,7 +969,7 @@ const PropertyInfo qdev_prop_link = { .create = create_link_property, }; -void qdev_property_add_static(DeviceState *dev, Property *prop) +void qdev_property_add_static(DeviceState *dev, const Property *prop) { Object *obj = OBJECT(dev); ObjectProperty *op; @@ -980,7 +980,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) field_prop_getter(prop->info), field_prop_setter(prop->info), prop->info->release, - prop); + (Property *)prop); object_property_set_description(obj, prop->name, prop->info->description); @@ -994,7 +994,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop) } static void qdev_class_add_property(DeviceClass *klass, const char *name, - Property *prop) + const Property *prop) { ObjectClass *oc = OBJECT_CLASS(klass); ObjectProperty *op; @@ -1007,7 +1007,7 @@ static void qdev_class_add_property(DeviceClass *klass, const char *name, field_prop_getter(prop->info), field_prop_setter(prop->info), prop->info->release, - prop); + (Property *)prop); } if (prop->set_default) { prop->info->set_default_value(op, prop); @@ -1046,7 +1046,7 @@ static void qdev_get_legacy_property(Object *obj, Visitor *v, * Do not use this in new code! QOM Properties added through this interface * will be given names in the "legacy" namespace. */ -static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop) +static void qdev_class_add_legacy_property(DeviceClass *dc, const Property *prop) { g_autofree char *name = NULL; @@ -1058,12 +1058,12 @@ static void qdev_class_add_legacy_property(DeviceClass *dc, Property *prop) name = g_strdup_printf("legacy-%s", prop->name); object_class_property_add(OBJECT_CLASS(dc), name, "str", prop->info->print ? qdev_get_legacy_property : prop->info->get, - NULL, NULL, prop); + NULL, NULL, (Property *)prop); } -void device_class_set_props(DeviceClass *dc, Property *props) +void device_class_set_props(DeviceClass *dc, const Property *props) { - Property *prop; + const Property *prop; dc->props_ = props; for (prop = props; prop && prop->name; prop++) { diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index 44994ea0e16..c346ea6ae4b 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -751,7 +751,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp) #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## __VA_ARGS__) -static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props, +static void qdev_print_props(Monitor *mon, DeviceState *dev, const Property *props, int indent) { if (!props)
Constify all accesses to qdev properties, except for the ObjectPropertyAccessor itself. This makes it possible to place them in read-only memory, and also lets Rust bindings switch from "static mut" arrays to "static"; which is advantageous, because mutable statics are highly discouraged. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- include/hw/qdev-core.h | 4 ++-- include/hw/qdev-properties.h | 4 ++-- hw/core/qdev-properties.c | 26 +++++++++++++------------- system/qdev-monitor.c | 2 +- 4 files changed, 18 insertions(+), 18 deletions(-)