Re: [Qemu-arm] [PATCH 2/3] qdev: support properties which don't set a default value

[Marc-André Lureau]

Hi

----- Original Message -----
> In some situations it's useful to have a qdev property which doesn't
> automatically set its default value when qdev_property_add_static is
> called (for instance when the default value is not constant).
> 
> Support this by adding a flag to the Property struct indicating
> whether to set the default value.  This replaces the existing test
> for whether the PorpertyInfo set_default_value function pointer is
> NULL, and we set the .set_default field to true for all those cases
> of struct Property which use a PropertyInfo with a non-NULL
> set_default_value, so behaviour remains the same as before.
> 
> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
> of wanting to set an integer property with no default value.
> 
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

minor comment below

> ---
> Since the previous patch fixed the only case that was
> using a default value but not explicitly setting .defval.[ui],
> this patch is a bit easier to review: .set_default = true
> is added in exactly the places that initialize .defval.[ui].
> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 20 ++++++++++++++++++++
>  hw/core/qdev.c               |  2 +-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9d7c1c0..d110163 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,6 +226,7 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    bool         set_default;
>      union {
>          int64_t i;
>          uint64_t u;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 36d040c..66816a5 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> +        .set_default = true,                                            \
>          .defval.i  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) {
> \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
>          .name      = (_name),                                    \
>          .info      = &(qdev_prop_bit),                           \
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> +        .set_default = true,                                     \
>          .defval.u  = (bool)_defval,                              \
>          }
>  
> @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type, typeof_field(_state, _field)),          \
> +        .set_default = true,                                            \
>          .defval.u  = (_type)_defval,                                    \
>          }
>  
> +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type)
> { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        }
> +
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
>          .name      = (_name),                                           \
>          .info      = &(qdev_prop_bit64),                                \
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> +        .set_default = true,                                            \
>          .defval.u  = (bool)_defval,                                     \
>          }
>  
> @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> +        .set_default = true,                                     \
>          .defval.u    = (bool)_defval,                            \
>          }
>  
> @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen;
>                            _arrayfield, _arrayprop, _arraytype) {        \
>          .name = (PROP_ARRAY_LEN_PREFIX _name),                          \
>          .info = &(qdev_prop_arraylen),                                  \
> +        .set_default = true,                                            \
>          .defval.u = 0,                                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 849952a..96965a7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property
> *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->info->set_default_value) {
> +    if (prop->set_default) {

Not sure if it's worth to have an assert(prop->info->set_default_value), 
probably not necessary.

>          prop->info->set_default_value(obj, prop);
>      }
>  }
> --
> 2.7.4
> 
>