Re: [PATCH v2 10/53] qom: add QAPI-aware property registration

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