Re: [Qemu-devel] [PATCH v2 05/45] qdev: remove PropertyInfo.qtype field

[Markus Armbruster]

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

> Remove dependency on qapi qtype, replace a field by a few PropertyInfo
> callbacks to set the default value type (introduced in commit 4f2d3d7).
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/hw/qdev-core.h       |  2 +-
>  include/hw/qdev-properties.h |  5 -----
>  hw/core/qdev-properties.c    | 35 ++++++++++++++++++++++++++++++++++-
>  hw/core/qdev.c               | 13 ++-----------
>  4 files changed, 37 insertions(+), 18 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e69489ec6c..9523339762 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -226,7 +226,6 @@ struct Property {
>      PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> -    QType        qtype;
>      int64_t      defval;
>      int          arrayoffset;
>      PropertyInfo *arrayinfo;
> @@ -238,6 +237,7 @@ struct PropertyInfo {
>      const char *description;
>      const char * const *enum_table;
>      int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
> +    void (*set_default_value)(Object *obj, const Property *prop);
>      ObjectPropertyAccessor *get;
>      ObjectPropertyAccessor *set;
>      ObjectPropertyRelease *release;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index d206fc93dd..85e68998a9 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -42,7 +42,6 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(_prop),                                          \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(_type,typeof_field(_state, _field)),           \
> -        .qtype     = QTYPE_QINT,                                        \
>          .defval    = (_type)_defval,                                    \
>          }
>  #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
> @@ -51,7 +50,6 @@ extern PropertyInfo qdev_prop_arraylen;
>          .bitnr    = (_bit),                                      \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(uint32_t,typeof_field(_state, _field)), \
> -        .qtype     = QTYPE_QBOOL,                                \
>          .defval    = (bool)_defval,                              \
>          }
>  #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
> @@ -60,7 +58,6 @@ extern PropertyInfo qdev_prop_arraylen;
>          .bitnr    = (_bit),                                             \
>          .offset    = offsetof(_state, _field)                           \
>              + type_check(uint64_t, typeof_field(_state, _field)),       \
> -        .qtype     = QTYPE_QBOOL,                                       \
>          .defval    = (bool)_defval,                                     \
>          }
>  
> @@ -69,7 +66,6 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info      = &(qdev_prop_bool),                          \
>          .offset    = offsetof(_state, _field)                    \
>              + type_check(bool, typeof_field(_state, _field)),    \
> -        .qtype     = QTYPE_QBOOL,                                \
>          .defval    = (bool)_defval,                              \
>          }
>  
> @@ -105,7 +101,6 @@ extern PropertyInfo qdev_prop_arraylen;
>          .info = &(qdev_prop_arraylen),                                  \
>          .offset = offsetof(_state, _field)                              \
>              + type_check(uint32_t, typeof_field(_state, _field)),       \
> -        .qtype = QTYPE_QINT,                                            \
>          .arrayinfo = &(_arrayprop),                                     \
>          .arrayfieldsize = sizeof(_arraytype),                           \
>          .arrayoffset = offsetof(_state, _arrayfield),                   \
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index fa3617db2d..482efb2683 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -69,6 +69,12 @@ static void set_enum(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>      visit_type_enum(v, prop->name, ptr, prop->info->enum_table, errp);
>  }
>  
> +static void set_default_value_enum(Object *obj, const Property *prop)
> +{
> +    object_property_set_str(obj, prop->info->enum_table[prop->defval],
> +                            prop->name, &error_abort);
> +}
> +
>  /* Bit */
>  
>  static uint32_t qdev_get_prop_mask(Property *prop)
> @@ -120,11 +126,17 @@ static void prop_set_bit(Object *obj, Visitor *v, const 
> char *name,
>      bit_prop_set(dev, prop, value);
>  }
>  
> +static void set_default_value_bool(Object *obj, const Property *prop)
> +{
> +    object_property_set_bool(obj, prop->defval, prop->name, &error_abort);
> +}
> +
>  PropertyInfo qdev_prop_bit = {
>      .name  = "bool",
>      .description = "on/off",
>      .get   = prop_get_bit,
>      .set   = prop_set_bit,
> +    .set_default_value = set_default_value_bool,
>  };
>  
>  /* Bit64 */
> @@ -183,6 +195,7 @@ PropertyInfo qdev_prop_bit64 = {
>      .description = "on/off",
>      .get   = prop_get_bit64,
>      .set   = prop_set_bit64,
> +    .set_default_value = set_default_value_bool,
>  };
>  
>  /* --- bool --- */
> @@ -216,6 +229,7 @@ PropertyInfo qdev_prop_bool = {
>      .name  = "bool",
>      .get   = get_bool,
>      .set   = set_bool,
> +    .set_default_value = set_default_value_bool,
>  };
>  
>  /* --- 8bit integer --- */
> @@ -245,10 +259,16 @@ static void set_uint8(Object *obj, Visitor *v, const 
> char *name, void *opaque,
>      visit_type_uint8(v, name, ptr, errp);
>  }
>  
> +static void set_default_value_int(Object *obj, const Property *prop)
> +{
> +    object_property_set_int(obj, prop->defval, prop->name, &error_abort);
> +}
> +
>  PropertyInfo qdev_prop_uint8 = {
>      .name  = "uint8",
>      .get   = get_uint8,
>      .set   = set_uint8,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- 16bit integer --- */
> @@ -282,6 +302,7 @@ PropertyInfo qdev_prop_uint16 = {
>      .name  = "uint16",
>      .get   = get_uint16,
>      .set   = set_uint16,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- 32bit integer --- */
> @@ -340,12 +361,14 @@ PropertyInfo qdev_prop_uint32 = {
>      .name  = "uint32",
>      .get   = get_uint32,
>      .set   = set_uint32,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  PropertyInfo qdev_prop_int32 = {
>      .name  = "int32",
>      .get   = get_int32,
>      .set   = set_int32,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- 64bit integer --- */
> @@ -379,6 +402,7 @@ PropertyInfo qdev_prop_uint64 = {
>      .name  = "uint64",
>      .get   = get_uint64,
>      .set   = set_uint64,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- string --- */
> @@ -526,6 +550,7 @@ PropertyInfo qdev_prop_on_off_auto = {
>      .enum_table = OnOffAuto_lookup,
>      .get = get_enum,
>      .set = set_enum,
> +    .set_default_value = set_default_value_enum,
>  };
>  
>  /* --- lost tick policy --- */
> @@ -537,6 +562,7 @@ PropertyInfo qdev_prop_losttickpolicy = {
>      .enum_table  = LostTickPolicy_lookup,
>      .get   = get_enum,
>      .set   = set_enum,
> +    .set_default_value = set_default_value_enum,
>  };
>  
>  /* --- Block device error handling policy --- */
> @@ -550,6 +576,7 @@ PropertyInfo qdev_prop_blockdev_on_error = {
>      .enum_table = BlockdevOnError_lookup,
>      .get = get_enum,
>      .set = set_enum,
> +    .set_default_value = set_default_value_enum,
>  };
>  
>  /* --- BIOS CHS translation */
> @@ -563,6 +590,7 @@ PropertyInfo qdev_prop_bios_chs_trans = {
>      .enum_table = BiosAtaTranslation_lookup,
>      .get = get_enum,
>      .set = set_enum,
> +    .set_default_value = set_default_value_enum,
>  };
>  
>  /* --- FDC default drive types */
> @@ -573,7 +601,8 @@ PropertyInfo qdev_prop_fdc_drive_type = {
>                     "144/288/120/none/auto",
>      .enum_table = FloppyDriveType_lookup,
>      .get = get_enum,
> -    .set = set_enum
> +    .set = set_enum,
> +    .set_default_value = set_default_value_enum,
>  };
>  
>  /* --- pci address --- */
> @@ -648,6 +677,7 @@ PropertyInfo qdev_prop_pci_devfn = {
>      .print = print_pci_devfn,
>      .get   = get_int32,
>      .set   = set_pci_devfn,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- blocksize --- */
> @@ -695,6 +725,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,
>  };
>  
>  /* --- pci host address --- */
> @@ -917,6 +948,7 @@ PropertyInfo qdev_prop_arraylen = {
>      .name = "uint32",
>      .get = get_uint32,
>      .set = set_prop_arraylen,
> +    .set_default_value = set_default_value_int,
>  };
>  
>  /* --- public helpers --- */
> @@ -1153,4 +1185,5 @@ PropertyInfo qdev_prop_size = {
>      .name  = "size",
>      .get = get_size,
>      .set = set_size,
> +    .set_default_value = set_default_value_int,
>  };
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 71ff95fd71..6674e11840 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -794,17 +794,8 @@ void qdev_property_add_static(DeviceState *dev, Property 
> *prop,
>                                      prop->info->description,
>                                      &error_abort);
>  
> -    if (prop->qtype == QTYPE_NONE) {
> -        return;
> -    }
> -
> -    if (prop->qtype == QTYPE_QBOOL) {
> -        object_property_set_bool(obj, prop->defval, prop->name, 
> &error_abort);
> -    } else if (prop->info->enum_table) {
> -        object_property_set_str(obj, prop->info->enum_table[prop->defval],
> -                                prop->name, &error_abort);
> -    } else if (prop->qtype == QTYPE_QINT) {
> -        object_property_set_int(obj, prop->defval, prop->name, &error_abort);
> +    if (prop->info->set_default_value) {
> +        prop->info->set_default_value(obj, prop);
>      }
>  }

Lovely cleanup.

The interesting part is the move of the bits controlling use of ->defval
from Property member .qtype (set only by DEFINE_PROP_foo() macros) to
its PropertyInfo method .info->set_default_value().  No functional
change if (1) we preserve existing use of ->defval and (2) we don't add
new uses of ->defval, or at least not uses that matter.

Direction (1) is obvious for DEFINE_PROP_BIT(), DEFINE_PROP_BIT64(),
DEFINE_PROP_BOOL().  They set .info to &qdev_prop_bit, &qdev_prop_bit64,
&qdev_prop_bool, respectively.  These all get .set_default_value =
set_default_value_bool, which preserves the effect of .qtype =
QTYPE_BOOL.  Good.

DEFINE_PROP_DEFAULT() sets .info to point to its argument _prop.  The
following arguments occur:

* In include/hw/qdev-properties.h: qdev_prop_uint8, qdev_prop_uint16,
  qdev_prop_uint32, qdev_prop_int32, qdev_prop_uint64, qdev_prop_size,
  qdev_prop_pci_devfn, qdev_prop_on_off_auto, qdev_prop_losttickpolicy,
  qdev_prop_blockdev_on_error, qdev_prop_bios_chs_trans,
  qdev_prop_blocksize

* In hw/block/fdc.c: qdev_prop_fdc_drive_type

* In hw/net/e1000e.c: local copies of qdev_prop_uint8, qdev_prop_uint16.

To preserve the effect of .qtype = QTYPE_QINT, these PropertyInfo need
.set_default_value = set_default_value_int or .set_default_value =
set_default_value_enum, depending on .enum_table.  They get it.  Good.

DEFINE_PROP_ARRAY() sets .info to &qdev_prop_arraylen.  Needs and gets
.set_default_value = set_default_value_int.  Good.

DEFINE_PROP_ARRAY() also takes a PropertyInfo argument, but it's not
relevant here, as the .qtype you remove doesn't apply to it.  I'm
mentioning this, because it confused me briefly.

Direction (2) isn't as visible in the patch.  For each PropertyInfo you
change, we need to find and check stores into .info not wrapped in the
DEFINE_PROP_foo() macros you change.

I can't find any.  Good.

Reviewed-by: Markus Armbruster <address@hidden>