Hi
On Fri, Aug 7, 2026 at 3:46 PM Markus Armbruster <[email protected]> wrote:
>
> Marc-André Lureau <[email protected]> writes:
>
> > Add object{_class}_property_add_qapi_enum(), and
> > object_{_class}_property_add_qapi() functions. Set the qapi_type pointer
> > on the resulting ObjectProperty.
> >
> > Signed-off-by: Marc-André Lureau <[email protected]>
>
> The patch appars to do two things: general QAPI-aware property add, and
> special enum property add. Would it make sense to split it?
>
sure
> > ---
> > include/qom/object.h | 105 +++++++++++++++++++++++++++++++++++++++++
> > qom/object.c | 131
> > +++++++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 232 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index d96a9afe21d..ebfdbff9a94 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1223,6 +1223,15 @@ void object_property_set_default_bool(ObjectProperty
> > *prop, bool value);
> > */
> > void object_property_set_default_str(ObjectProperty *prop, const char
> > *value);
> >
> > +/**
> > + * object_property_set_default_enum:
> > + * @prop: the property to set
> > + * @value: the value to be written to the property
> > + *
> > + * Set the property default value.
> > + */
> > +void object_property_set_default_enum(ObjectProperty *prop, int value);
> > +
>
> This addition isn't mentioned in the commit message, unlike the others.
> So the patch actually does three things.
>
> Peeking ahead to find users... there's just one, in PATCH 12. It
> replaces object_property_set_default_str() there.
>
> > /**
> > * object_property_set_default_list:
> > * @prop: the property to set
> > @@ -1915,6 +1924,102 @@ ObjectProperty
> > *object_class_property_add_enum(ObjectClass *klass,
> > int (*get)(Object *, Error **),
> > void (*set)(Object *, int, Error **));
> >
> > +/**
> > + * struct QapiEnumProp - Descriptor for a QOM property backed by a QAPI
> > enum type
> > + *
> > + * Binds a QOM object property to a QAPI enum, providing automatic
> > + * string<->int conversion through QAPI visitors and optional
> > + * default-value initialization during object instance init.
> > + *
> > + * Use the QAPI_ENUM_PROP() macro to construct instances inline.
> > + *
> > + * @name: property name exposed on the QOM object
> > + * @description: human-readable description (shown in ``-device help``,
> > etc.)
> > + * @default_value: initial enum value applied via @set during instance
> > init,
> > + * or -1 (the QAPI_ENUM_PROP default) to skip
> > initialization
> > + * @qapi_type: pointer to the generated QAPITypeInfo for the enum
> > + * (provides the string<->int lookup table)
> > + * @get: getter that returns the current enum value as int, or -1 on error
> > + * @set: setter that receives the enum value as int; %NULL for read-only
> > props
> > + */
> > +typedef struct QapiEnumProp {
> > + const char *name;
> > + const char *description;
> > + const int default_value;
> > + const QAPITypeInfo *qapi_type;
> > + int (*get)(Object *, Error **);
> > + void (*set)(Object *, int, Error **);
> > +} QapiEnumProp;
> > +
> > +#define QAPI_ENUM_PROP(...) ({ \
> > + static const QapiEnumProp _prop = { \
> > + .default_value = -1, __VA_ARGS__ \
> > + }; \
> > + &_prop; })
> > +
> > +/**
> > + * object_property_add_qapi_enum:
> > + * @obj: the object to add a property to
> > + * @prop: property descriptor
> > + *
> > + * Add an enum property with QAPI type association.
> > + *
> > + * Use the QAPI_ENUM_PROP() macro to construct the property descriptor
> > + * inline. If .default_value is not set, the property is not initialized
> > + * (default_value is -1). Otherwise, the setter is called with
> > + * default_value during object instance init.
> > + *
> > + * Example::
> > + *
> > + * object_class_property_add_qapi_enum(oc, QAPI_ENUM_PROP(
> > + * .name = "policy",
> > + * .description = "Set the NUMA policy",
> > + * .default_value = HOST_MEM_POLICY_DEFAULT,
> > + * .qapi_type = &HostMemPolicy_type_info,
> > + * .get = my_get_policy,
> > + * .set = my_set_policy,
> > + * ));
>
> I'm not sure the macro is a good idea. Could we use a simple compound
> literal instead?
A plain compound literal has automatic storage.. the macro just eases
this. You can always open code if it helps.
>
> > + *
> > + * Returns: The newly added property on success, or %NULL on failure.
> > + */
> > +ObjectProperty *
> > +object_property_add_qapi_enum(Object *obj, const QapiEnumProp *prop);
> > +
> > +ObjectProperty *
> > +object_class_property_add_qapi_enum(ObjectClass *klass, const QapiEnumProp
> > *prop);
>
> Observation, not pointing out flaws in your work: I still have to write
> a .get and a .set in order to define a property, and most of these are
> 100% stupid. I wish I could simply specify "read/write this member" and
> be done, like in qdev.
>
> Are there enum properties whose type is *not* a QAPI enum type? Hmm we
> do have enum-like string properties, e.g. "boot-mode" in
> hw/arm/xilinx_zynq.c, and "riscv-aia" in target/riscv/kvm/kvm-cpu.c.
> Not a fan.
>
> Should only these functions be used to define enum properties going
> forward? If yes, the _qapi part in their name is redundant (but might
> be desirable for consistency, I don't know).
>
> > +
> > +/**
> > + * object_property_add_qapi:
> > + * @obj: the object to add a property to
> > + * @name: the name of the property
> > + * @qapi_type: QAPI type info descriptor
> > + * @get: the getter or %NULL if the property is write-only.
>
> Either end all your descriptions with punctuation, or none.
fixed
>
> > + * @set: the setter or %NULL if the property is read-only
> > + * @release: called when the property is removed from the object
> > + * @opaque: opaque pointer for get/set/release
> > + *
> > + * Add a property with a QAPI type association. The property type name
> > + * is derived from @qapi_type->name.
> > + *
> > + * Returns: The newly added property on success, or %NULL on failure.
> > + */
> > +ObjectProperty *
> > +object_property_add_qapi(Object *obj, const char *name,
> > + const QAPITypeInfo *qapi_type,
> > + ObjectPropertyAccessor *get,
> > + ObjectPropertyAccessor *set,
> > + ObjectPropertyRelease *release,
> > + void *opaque);
>
> This is like object_property_add() with char *type replaced by
> QAPITypeInfo *qapi_type. Good.
>
> Not so good: the argument descriptions in the function comment differ.
> Let's pick the best one, and use it everywhere.
fixed
>
> > +
> > +ObjectProperty *
> > +object_class_property_add_qapi(ObjectClass *klass,
> > + const char *name,
> > + const QAPITypeInfo *qapi_type,
> > + ObjectPropertyAccessor *get,
> > + ObjectPropertyAccessor *set,
> > + ObjectPropertyRelease *release,
> > + void *opaque);
>
> Likewise, except there's no function comment. Can't fault your patch
> for that; the existing class property functions lack comments, too. I
> do fault the code before your series, though: we have pairs of functions
> where we want people to use one, and document only the other. Perhaps
> we could figure out how to transmit mild electric shocks via the
> keyboard to further deter the use of the better one?
>
> Do we want people to use QAPI-aware functions like these to define
> properties whenever possible? I figure we do. What about telling
> people in the comments?
>
> Function to add properties always come in pairs, one for properties tied
> to the class, and one for properties tied to the object. Most users of
> the latter should use the former instead. Questions for the QOM
> maintainers:
>
> 1. Is it time to stop requiring the latter?
> object_property_add_qapi_enum() remains unused at the end of the
> series...
>
> 2. Should the preferred one at least have a shorter name than the other
> one?
There is certainly room for future improvements, I don't try to
address everything at once :)
>
> > +
> > /**
> > * object_property_add_tm:
> > * @obj: the object to add a property to
>
> Just skimming the remainder for now.
>
> > diff --git a/qom/object.c b/qom/object.c
> > index a085e78557f..e5e2f09a0a2 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -24,6 +24,7 @@
> > #include "qapi/forward-visitor.h"
> > #include "qapi/qapi-builtin-visit.h"
> > #include "qobject/qdict.h"
> > +#include "qapi/qapi-type-info.h"
> > #include "qobject/qjson.h"
> > #include "qemu/id.h"
> > #include "qapi/qmp/qerror.h"
> > @@ -1717,6 +1718,15 @@ void object_property_set_default_str(ObjectProperty
> > *prop, const char *value)
> > object_property_set_default(prop, QOBJECT(qstring_from_str(value)));
> > }
> >
> > +void object_property_set_default_enum(ObjectProperty *prop, int value)
> > +{
> > + assert(prop && prop->qapi_type && prop->qapi_type->lookup);
> > +
> > + object_property_set_default(prop, QOBJECT(qstring_from_str(
> > + qapi_enum_lookup(prop->qapi_type->lookup, value)
> > + )));
> > +}
> > +
> > void object_property_set_default_list(ObjectProperty *prop)
> > {
> > object_property_set_default(prop, QOBJECT(qlist_new()));
> > @@ -1775,7 +1785,6 @@ int object_property_get_enum(Object *obj, const char
> > *name,
> > char *str;
> > int ret;
> > ObjectProperty *prop = object_property_find_err(obj, name, errp);
> > - EnumProperty *enumprop;
> >
> > if (prop == NULL) {
> > return -1;
> > @@ -1788,14 +1797,17 @@ int object_property_get_enum(Object *obj, const
> > char *name,
> > return -1;
> > }
> >
> > - enumprop = prop->opaque;
> > -
> > str = object_property_get_str(obj, name, errp);
> > if (!str) {
> > return -1;
> > }
> >
> > - ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
> > + if (prop->qapi_type) {
> > + ret = qapi_enum_parse(prop->qapi_type->lookup, str, -1, errp);
> > + } else {
> > + EnumProperty *enumprop = prop->opaque;
> > + ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
> > + }
> > g_free(str);
> >
> > return ret;
> > @@ -2598,6 +2610,117 @@ object_class_property_add_enum(ObjectClass *klass,
> > const char *name,
> > prop);
> > }
> >
> > +static void get_qapi_enum(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + const QapiEnumProp *prop = opaque;
> > + int value;
> > + Error *err = NULL;
> > +
> > + value = prop->get(obj, &err);
> > + if (err) {
> > + error_propagate(errp, err);
> > + return;
> > + }
> > +
> > + visit_type_enum(v, name, &value, prop->qapi_type->lookup, errp);
> > +}
> > +
> > +static void set_qapi_enum(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
> > +{
> > + const QapiEnumProp *prop = opaque;
> > + int value;
> > +
> > + if (!visit_type_enum(v, name, &value, prop->qapi_type->lookup, errp)) {
> > + return;
> > + }
> > + prop->set(obj, value, errp);
> > +}
> > +
> > +static void init_qapi_enum(Object *obj, ObjectProperty *prop)
> > +{
> > + const QapiEnumProp *e = prop->opaque;
> > +
> > + if (e->set && e->default_value >= 0) {
> > + e->set(obj, e->default_value, &error_abort);
> > + }
> > +}
> > +
> > +ObjectProperty *
> > +object_property_add_qapi_enum(Object *obj, const QapiEnumProp *e)
> > +{
> > + ObjectProperty *prop;
> > +
> > + assert(e && e->qapi_type && e->qapi_type->lookup);
> > +
> > + prop = object_property_add(obj, e->name, e->qapi_type->name,
> > + e->get ? get_qapi_enum : NULL,
> > + e->set ? set_qapi_enum : NULL,
> > + NULL,
> > + (void *)e);
> > + prop->qapi_type = e->qapi_type;
>
> Shouldn't this wrap around object_property_add_qapi()?
ok
>
> > + prop->description = g_strdup(e->description);
> > + if (e->default_value >= 0) {
> > + prop->init = init_qapi_enum;
> > + }
> > +
> > + return prop;
> > +}
> > +
> > +ObjectProperty *
> > +object_class_property_add_qapi_enum(ObjectClass *klass, const QapiEnumProp
> > *e)
> > +{
> > + ObjectProperty *prop;
> > +
> > + assert(e && e->qapi_type && e->qapi_type->lookup);
> > +
> > + prop = object_class_property_add(klass, e->name, e->qapi_type->name,
> > + e->get ? get_qapi_enum : NULL,
> > + e->set ? set_qapi_enum : NULL,
> > + NULL,
> > + (void *)e);
> > + prop->qapi_type = e->qapi_type;
>
> Shouldn't this wrap around object_class_property_add_qapi()?
>
> > + prop->description = g_strdup(e->description);
> > + if (e->default_value >= 0) {
> > + prop->init = init_qapi_enum;
> > + }
> > +
> > + return prop;
> > +}
> > +
> > +ObjectProperty *
> > +object_property_add_qapi(Object *obj, const char *name,
> > + const QAPITypeInfo *qapi_type,
> > + ObjectPropertyAccessor *get,
> > + ObjectPropertyAccessor *set,
> > + ObjectPropertyRelease *release,
> > + void *opaque)
> > +{
> > + ObjectProperty *prop;
> > +
> > + prop = object_property_add(obj, name, qapi_type->name,
> > + get, set, release, opaque);
> > + prop->qapi_type = qapi_type;
> > + return prop;
> > +}
> > +
> > +ObjectProperty *
> > +object_class_property_add_qapi(ObjectClass *klass, const char *name,
> > + const QAPITypeInfo *qapi_type,
> > + ObjectPropertyAccessor *get,
> > + ObjectPropertyAccessor *set,
> > + ObjectPropertyRelease *release,
> > + void *opaque)
> > +{
> > + ObjectProperty *prop;
> > +
> > + prop = object_class_property_add(klass, name, qapi_type->name,
> > + get, set, release, opaque);
> > + prop->qapi_type = qapi_type;
> > + return prop;
> > +}
> > +
> > typedef struct TMProperty {
> > void (*get)(Object *, struct tm *, Error **);
> > } TMProperty;
>
