Re: [PATCH v2 8/8] qom: Use qlit to represent property defaults

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 15964 bytes --]

On Tue, Nov 17, 2020 at 2:45 AM Eduardo Habkost <ehabkost@redhat.com> wrote:

> Using QLitObject lets us get rid of most of the
> .set_default_value functions, and just use
> object_property_set_default() directly.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
> Changes v1 -> v2:
> * Instead of initializing defval to QLIT_QNULL
>   by default, just check for QTYPE_NONE, to find out if .defval
>   was explicitly set.  This avoids extra complexity at
>   set_prop_arraylen().
> ---
>  include/hw/qdev-properties-system.h   |  2 +-
>  include/qom/field-property-internal.h |  4 ---
>  include/qom/field-property.h          | 26 ++++++++-----------
>  include/qom/property-types.h          | 19 ++++++--------
>  hw/core/qdev-properties-system.c      |  8 ------
>  qom/field-property.c                  | 27 ++++++++++++++------
>  qom/property-types.c                  | 36 ++++-----------------------
>  7 files changed, 42 insertions(+), 80 deletions(-)
>
> diff --git a/include/hw/qdev-properties-system.h
> b/include/hw/qdev-properties-system.h
> index 0ac327ae60..a586424a33 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -65,7 +65,7 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>
>  #define DEFINE_PROP_UUID(_name, _state, _field) \
>      DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID, \
> -                .set_default = true)
> +                .defval = QLIT_QSTR("auto"))
>
>  #define DEFINE_PROP_AUDIODEV(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_audiodev, QEMUSoundCard)
> diff --git a/include/qom/field-property-internal.h
> b/include/qom/field-property-internal.h
> index a7b7e2b69d..9bc29e9b67 100644
> --- a/include/qom/field-property-internal.h
> +++ b/include/qom/field-property-internal.h
> @@ -15,10 +15,6 @@ void field_prop_set_enum(Object *obj, Visitor *v, const
> char *name,
>
>  void field_prop_set_default_value_enum(ObjectProperty *op,
>                                         const Property *prop);
> -void field_prop_set_default_value_int(ObjectProperty *op,
> -                                      const Property *prop);
> -void field_prop_set_default_value_uint(ObjectProperty *op,
> -                                       const Property *prop);
>
>  void field_prop_get_int32(Object *obj, Visitor *v, const char *name,
>                            void *opaque, Error **errp);
> diff --git a/include/qom/field-property.h b/include/qom/field-property.h
> index 0cb1fe2217..3cfd19cc14 100644
> --- a/include/qom/field-property.h
> +++ b/include/qom/field-property.h
> @@ -6,6 +6,7 @@
>
>  #include "qom/object.h"
>  #include "qapi/util.h"
> +#include "qapi/qmp/qlit.h"
>
>  /**
>   * struct Property: definition of a field property
> @@ -27,21 +28,8 @@ struct Property {
>      const PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> -    /**
> -     * @set_default: true if the default value should be set from @defval,
> -     *    in which case @info->set_default_value must not be NULL
> -     *    (if false then no default value is set by the property system
> -     *     and the field retains whatever value it was given by
> instance_init).
> -     */
> -    bool         set_default;
> -    /**
> -     * @defval: default value for the property. This is used only if
> @set_default
> -     *     is true.
> -     */
> -    union {
> -        int64_t i;
> -        uint64_t u;
> -    } defval;
> +    /** @defval: If not QTYPE_NONE, the default value for the property */
> +    QLitObject defval;
>      /* private: */
>      int          arrayoffset;
>      const PropertyInfo *arrayinfo;
> @@ -61,7 +49,13 @@ struct PropertyInfo {
>      const QEnumLookup *enum_table;
>      /** @print: String formatting function, for the human monitor */
>      int (*print)(Object *obj, Property *prop, char *dest, size_t len);
> -    /** @set_default_value: Callback for initializing the default value */
> +    /**
> +     * @set_default_value: Optional callback for initializing the default
> value
> +     *
> +     * Most property types don't need to set this, as by default
> +     * object_property_set_default() is called with the value at
> +     * Property.defval.
> +     */
>      void (*set_default_value)(ObjectProperty *op, const Property *prop);
>      /** @create: Optional callback for creation of property */
>      ObjectProperty *(*create)(ObjectClass *oc, const char *name,
> diff --git a/include/qom/property-types.h b/include/qom/property-types.h
> index 3132ddafd9..869d1a993a 100644
> --- a/include/qom/property-types.h
> +++ b/include/qom/property-types.h
> @@ -5,6 +5,7 @@
>  #define QOM_PROPERTY_TYPES_H
>
>  #include "qom/field-property.h"
> +#include "qapi/qmp/qlit.h"
>
>  extern const PropertyInfo prop_info_bit;
>  extern const PropertyInfo prop_info_bit64;
> @@ -25,34 +26,29 @@ extern const PropertyInfo prop_info_link;
>
>  #define PROP_SIGNED(_state, _field, _defval, _prop, _type, ...) \
>      FIELD_PROP(_state, _field, _prop, _type,                    \
> -               .set_default = true,                             \
> -               .defval.i    = (_type)_defval,                   \
> +               .defval = QLIT_QNUM_INT((_type)_defval),                \
>                 __VA_ARGS__)
>
>  #define PROP_UNSIGNED(_state, _field, _defval, _prop, _type, ...) \
>      FIELD_PROP(_state, _field, _prop, _type,                    \
> -               .set_default = true,                             \
> -               .defval.u  = (_type)_defval,                     \
> +               .defval = QLIT_QNUM_UINT((_type)_defval),               \
>                 __VA_ARGS__)
>
>  #define PROP_BIT(_state, _field, _bit, _defval, ...) \
>      FIELD_PROP(_state, _field, prop_info_bit, uint32_t,         \
>                 .bitnr       = (_bit),                           \
> -               .set_default = true,                             \
> -               .defval.u    = (bool)_defval,                    \
> +               .defval = QLIT_QBOOL(_defval),                   \
>                 __VA_ARGS__)
>
>  #define PROP_BIT64(_state, _field, _bit, _defval, ...) \
>      FIELD_PROP(_state, _field, prop_info_bit64, uint64_t,       \
>                 .bitnr    = (_bit),                              \
> -               .set_default = true,                             \
> -               .defval.u  = (bool)_defval,                      \
> +               .defval = QLIT_QBOOL(_defval),                   \
>                 __VA_ARGS__)
>
>  #define PROP_BOOL(_state, _field, _defval, ...) \
>      FIELD_PROP(_state, _field, prop_info_bool, bool,            \
> -               .set_default = true,                             \
> -               .defval.u    = (bool)_defval,                    \
> +               .defval = QLIT_QBOOL(_defval),                   \
>                 __VA_ARGS__)
>
>  #define PROP_LINK(_state, _field, _type, _ptr_type, ...) \
> @@ -131,8 +127,7 @@ extern const PropertyInfo prop_info_link;
>                            _arrayfield, _arrayprop, _arraytype) \
>      DEFINE_PROP((PROP_ARRAY_LEN_PREFIX _name),                 \
>                  _state, _field, prop_info_arraylen, uint32_t,  \
> -                .set_default = true,                           \
> -                .defval.u = 0,                                 \
> +                .defval = QLIT_QNUM_UINT(0),                   \
>                  .arrayinfo = &(_arrayprop),                    \
>                  .arrayfieldsize = sizeof(_arraytype),          \
>                  .arrayoffset = offsetof(_state, _arrayfield))
> diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> index 8da68f076c..d9be5372f6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -570,7 +570,6 @@ const PropertyInfo qdev_prop_blocksize = {
>                     " and " MAX_BLOCK_SIZE_STR,
>      .get   = field_prop_get_size32,
>      .set   = set_blocksize,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- Block device error handling policy --- */
> @@ -768,7 +767,6 @@ const PropertyInfo qdev_prop_pci_devfn = {
>      .print = print_pci_devfn,
>      .get   = field_prop_get_int32,
>      .set   = set_pci_devfn,
> -    .set_default_value = field_prop_set_default_value_int,
>  };
>
>  /* --- pci host address --- */
> @@ -1080,16 +1078,10 @@ static void set_uuid(Object *obj, Visitor *v,
> const char *name, void *opaque,
>      g_free(str);
>  }
>
> -static void set_default_uuid_auto(ObjectProperty *op, const Property
> *prop)
> -{
> -    object_property_set_default_str(op, UUID_VALUE_AUTO);
> -}
> -
>  const PropertyInfo qdev_prop_uuid = {
>      .name  = "str",
>      .description = "UUID (aka GUID) or \"" UUID_VALUE_AUTO
>          "\" for random value (default)",
>      .get   = get_uuid,
>      .set   = set_uuid,
> -    .set_default_value = set_default_uuid_auto,
>  };
> diff --git a/qom/field-property.c b/qom/field-property.c
> index cb729626ce..9cb5ded41a 100644
> --- a/qom/field-property.c
> +++ b/qom/field-property.c
> @@ -47,6 +47,20 @@ static ObjectPropertyAccessor *field_prop_setter(const
> PropertyInfo *info)
>      return info->set ? field_prop_set : NULL;
>  }
>
> +static void field_prop_set_default_value(ObjectProperty *op,
> +                                         Property *prop)
> +{
> +    if (qlit_type(&prop->defval) == QTYPE_NONE) {
> +        return;
> +    }
> +
> +    if (prop->info->set_default_value) {
> +        prop->info->set_default_value(op, prop);
> +    } else {
> +        object_property_set_default(op, qobject_from_qlit(&prop->defval));
> +    }
> +}
> +
>  /*
>   * Property release callback for dynamically-created properties:
>   * We call the underlying element's property release hook, and
> @@ -83,11 +97,9 @@ object_property_add_field(Object *obj, const char *name,
>      object_property_set_description(obj, name,
>                                      newprop->info->description);
>
> -    if (newprop->set_default) {
> -        newprop->info->set_default_value(op, newprop);
> -        if (op->init) {
> -            op->init(obj, op);
> -        }
> +    field_prop_set_default_value(op, prop);
> +    if (op->init) {
> +        op->init(obj, op);
>      }
>
>      op->allow_set = allow_set;
> @@ -113,9 +125,8 @@ object_class_property_add_field_static(ObjectClass
> *oc, const char *name,
>                                         prop->info->release,
>                                         prop);
>      }
> -    if (prop->set_default) {
> -        prop->info->set_default_value(op, prop);
> -    }
> +
> +    field_prop_set_default_value(op, prop);
>      if (prop->info->description) {
>          object_class_property_set_description(oc, name,
>                                                prop->info->description);
> diff --git a/qom/property-types.c b/qom/property-types.c
> index 4b3fe15b6a..fe96f1f49a 100644
> --- a/qom/property-types.c
> +++ b/qom/property-types.c
> @@ -28,8 +28,11 @@ void field_prop_set_enum(Object *obj, Visitor *v, const
> char *name,
>  void field_prop_set_default_value_enum(ObjectProperty *op,
>                                         const Property *prop)
>  {
> -    object_property_set_default_str(op,
> -        qapi_enum_lookup(prop->info->enum_table, prop->defval.i));
> +    QObject *defval = qobject_from_qlit(&prop->defval);
> +    const char *str = qapi_enum_lookup(prop->info->enum_table,
> +                                       qnum_get_int(qobject_to(QNum,
> defval)));
> +    object_property_set_default_str(op, str);
> +    qobject_unref(defval);
>  }
>
>  const PropertyInfo prop_info_enum = {
> @@ -80,17 +83,11 @@ static void prop_set_bit(Object *obj, Visitor *v,
> const char *name,
>      bit_prop_set(obj, prop, value);
>  }
>
> -static void set_default_value_bool(ObjectProperty *op, const Property
> *prop)
> -{
> -    object_property_set_default_bool(op, prop->defval.u);
> -}
> -
>  const PropertyInfo prop_info_bit = {
>      .name  = "bool",
>      .description = "on/off",
>      .get   = prop_get_bit,
>      .set   = prop_set_bit,
> -    .set_default_value = set_default_value_bool,
>  };
>
>  /* Bit64 */
> @@ -139,7 +136,6 @@ const PropertyInfo prop_info_bit64 = {
>      .description = "on/off",
>      .get   = prop_get_bit64,
>      .set   = prop_set_bit64,
> -    .set_default_value = set_default_value_bool,
>  };
>
>  /* --- bool --- */
> @@ -166,7 +162,6 @@ const PropertyInfo prop_info_bool = {
>      .name  = "bool",
>      .get   = get_bool,
>      .set   = set_bool,
> -    .set_default_value = set_default_value_bool,
>  };
>
>  /* --- 8bit integer --- */
> @@ -189,23 +184,10 @@ static void set_uint8(Object *obj, Visitor *v, const
> char *name, void *opaque,
>      visit_type_uint8(v, name, ptr, errp);
>  }
>
> -void field_prop_set_default_value_int(ObjectProperty *op,
> -                                      const Property *prop)
> -{
> -    object_property_set_default_int(op, prop->defval.i);
> -}
> -
> -void field_prop_set_default_value_uint(ObjectProperty *op,
> -                                       const Property *prop)
> -{
> -    object_property_set_default_uint(op, prop->defval.u);
> -}
> -
>  const PropertyInfo prop_info_uint8 = {
>      .name  = "uint8",
>      .get   = get_uint8,
>      .set   = set_uint8,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- 16bit integer --- */
> @@ -232,7 +214,6 @@ const PropertyInfo prop_info_uint16 = {
>      .name  = "uint16",
>      .get   = get_uint16,
>      .set   = set_uint16,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- 32bit integer --- */
> @@ -277,14 +258,12 @@ const PropertyInfo prop_info_uint32 = {
>      .name  = "uint32",
>      .get   = get_uint32,
>      .set   = set_uint32,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  const PropertyInfo prop_info_int32 = {
>      .name  = "int32",
>      .get   = field_prop_get_int32,
>      .set   = set_int32,
> -    .set_default_value = field_prop_set_default_value_int,
>  };
>
>  /* --- 64bit integer --- */
> @@ -329,14 +308,12 @@ const PropertyInfo prop_info_uint64 = {
>      .name  = "uint64",
>      .get   = get_uint64,
>      .set   = set_uint64,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  const PropertyInfo prop_info_int64 = {
>      .name  = "int64",
>      .get   = get_int64,
>      .set   = set_int64,
> -    .set_default_value = field_prop_set_default_value_int,
>  };
>
>  /* --- string --- */
> @@ -431,7 +408,6 @@ const PropertyInfo prop_info_size32 = {
>      .name  = "size",
>      .get = field_prop_get_size32,
>      .set = set_size32,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- support for array properties --- */
> @@ -495,7 +471,6 @@ const PropertyInfo prop_info_arraylen = {
>      .name = "uint32",
>      .get = get_uint32,
>      .set = set_prop_arraylen,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- 64bit unsigned int 'size' type --- */
> @@ -522,7 +497,6 @@ const PropertyInfo prop_info_size = {
>      .name  = "size",
>      .get = get_size,
>      .set = set_size,
> -    .set_default_value = field_prop_set_default_value_uint,
>  };
>
>  /* --- object link property --- */
> --
> 2.28.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 19243 bytes --]