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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>

The patch appars to do two things: general QAPI-aware property add, and
special enum property add.  Would it make sense to split it?

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

> + *
> + * 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.

> + * @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.

> +
> +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?

> +
>  /**
>   * 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()?

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