Re: [Qemu-devel] [PATCH v2 19/45] qdev: add unsigned properties

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Add and use unsigned type for various properties.
>
> Signed-off-by: Marc-André Lureau <address@hidden>

The commit message is a bit misleading.  We don't "add unsigned
properties", we clean up the property code to avoid type casts between
signed and unsigned.

I'm not sure that's worth the churn by itself.  But perhaps it helps
later in this series.  If yes, can you give me a hint?

> ---
>  include/hw/qdev-core.h       |  1 +
>  include/hw/qdev-properties.h | 51 
> +++++++++++++++++++++++++++-----------------
>  hw/core/qdev-properties.c    | 23 ++++++++++++--------
>  3 files changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 784971b8d8..9d7c1c0e9b 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -228,6 +228,7 @@ struct Property {
>      uint8_t      bitnr;
>      union {
>          int64_t i;
> +        uint64_t u;
>      } defval;
>      int          arrayoffset;
>      PropertyInfo *arrayinfo;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 6663a547ea..16d3ada8df 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -37,6 +37,7 @@ extern PropertyInfo qdev_prop_arraylen;
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(_type, typeof_field(_state, _field)),   \
>          }
> +
>  #define DEFINE_PROP_INT(_name, _state, _field, _defval, _prop, _type) { \
>          .name      = (_name),                                           \
>          .info      = &(_prop),                                          \
> @@ -44,29 +45,39 @@ extern PropertyInfo qdev_prop_arraylen;
>              + type_check(_type,typeof_field(_state, _field)),           \
>          .defval.i  = (_type)_defval,                                    \
>          }
> -#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)), \
> -        .defval.i  = (bool)_defval,                              \
> +
> +#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)),        \
> +        .defval.u   = (bool)_defval,                                    \
>          }

Let's keep the backslashes where they are to reduce churn.

> +
> +#define DEFINE_PROP_UINT(_name, _state, _field, _defval, _prop, _type) { \
> +        .name      = (_name),                                           \
> +        .info      = &(_prop),                                          \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(_type, typeof_field(_state, _field)),          \
> +        .defval.u  = (_type)_defval,                                    \
> +        }
> +
>  #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)),       \
> -        .defval.i  = (bool)_defval,                                     \
> +        .defval.u  = (bool)_defval,                                     \
>          }
>  
> -#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) {       \
> -        .name      = (_name),                                    \
> -        .info      = &(qdev_prop_bool),                          \
> -        .offset    = offsetof(_state, _field)                    \
> -            + type_check(bool, typeof_field(_state, _field)),    \
> -        .defval.i  = (bool)_defval,                              \
> +#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) {      \
> +        .name      = (_name),                                   \
> +        .info      = &(qdev_prop_bool),                         \
> +        .offset    = offsetof(_state, _field)                   \
> +            + type_check(bool, typeof_field(_state, _field)),   \
> +        .defval.u    = (bool)_defval,                           \
>          }

Likewise.

>  
>  #define PROP_ARRAY_LEN_PREFIX "len-"
> @@ -107,17 +118,17 @@ extern PropertyInfo qdev_prop_arraylen;
>          }
>  
>  #define DEFINE_PROP_UINT8(_n, _s, _f, _d)                       \
> -    DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)
>  #define DEFINE_PROP_UINT16(_n, _s, _f, _d)                      \
> -    DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t)
>  #define DEFINE_PROP_UINT32(_n, _s, _f, _d)                      \
> -    DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t)
>  #define DEFINE_PROP_INT32(_n, _s, _f, _d)                      \
>      DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_int32, int32_t)
>  #define DEFINE_PROP_UINT64(_n, _s, _f, _d)                      \
> -    DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
>  #define DEFINE_PROP_SIZE(_n, _s, _f, _d)                       \
> -    DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_size, uint64_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_size, uint64_t)
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
>      DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
>  
> @@ -163,7 +174,7 @@ extern PropertyInfo qdev_prop_arraylen;
>  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>      DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_INT(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UINT(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>  
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 952cda7758..92fb43cb0d 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -71,7 +71,7 @@ static void set_enum(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  
>  static void set_default_value_enum(Object *obj, const Property *prop)
>  {
> -    object_property_set_str(obj, prop->info->enum_table[prop->defval.i],
> +    object_property_set_str(obj, prop->info->enum_table[prop->defval.u],
>                              prop->name, &error_abort);
>  }
>  
> @@ -128,7 +128,7 @@ static void prop_set_bit(Object *obj, Visitor *v, const 
> char *name,
>  
>  static void set_default_value_bool(Object *obj, const Property *prop)
>  {
> -    object_property_set_bool(obj, prop->defval.i, prop->name, &error_abort);
> +    object_property_set_bool(obj, prop->defval.u, prop->name, &error_abort);
>  }
>  
>  PropertyInfo qdev_prop_bit = {
> @@ -264,11 +264,16 @@ static void set_default_value_int(Object *obj, const 
> Property *prop)
>      object_property_set_int(obj, prop->defval.i, prop->name, &error_abort);
>  }
>  
> +static void set_default_value_uint(Object *obj, const Property *prop)
> +{
> +    object_property_set_uint(obj, prop->defval.u, prop->name, &error_abort);
> +}
> +
>  PropertyInfo qdev_prop_uint8 = {
>      .name  = "uint8",
>      .get   = get_uint8,
>      .set   = set_uint8,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  /* --- 16bit integer --- */
> @@ -302,7 +307,7 @@ PropertyInfo qdev_prop_uint16 = {
>      .name  = "uint16",
>      .get   = get_uint16,
>      .set   = set_uint16,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  /* --- 32bit integer --- */
> @@ -361,7 +366,7 @@ PropertyInfo qdev_prop_uint32 = {
>      .name  = "uint32",
>      .get   = get_uint32,
>      .set   = set_uint32,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  PropertyInfo qdev_prop_int32 = {
> @@ -402,7 +407,7 @@ PropertyInfo qdev_prop_uint64 = {
>      .name  = "uint64",
>      .get   = get_uint64,
>      .set   = set_uint64,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  /* --- string --- */
> @@ -725,7 +730,7 @@ PropertyInfo qdev_prop_blocksize = {
>      .description = "A power of two between 512 and 32768",
>      .get   = get_uint16,
>      .set   = set_blocksize,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  /* --- pci host address --- */
> @@ -948,7 +953,7 @@ PropertyInfo qdev_prop_arraylen = {
>      .name = "uint32",
>      .get = get_uint32,
>      .set = set_prop_arraylen,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };
>  
>  /* --- public helpers --- */
> @@ -1185,5 +1190,5 @@ PropertyInfo qdev_prop_size = {
>      .name  = "size",
>      .get = get_size,
>      .set = set_size,
> -    .set_default_value = set_default_value_int,
> +    .set_default_value = set_default_value_uint,
>  };