Re: [Qemu-devel] [PATCH 03/16] qdev-properties: add PROP_TYPE_ENUM

[Markus Armbruster]

I haven't been able to follow the evolution of this series, my apologies
if I'm missing things already discussed.

Alon Levy <address@hidden> writes:

> Example usage:
>
> EnumTable foo_enum_table[] = {
>     {"bar", 1},
>     {"buz", 2},
>     {NULL, 0},
> };
>
> DEFINE_PROP_ENUM("foo", State, foo, 1, foo_enum_table)
>
> When using qemu -device foodev,? it will appear as:
>  foodev.foo=bar/buz
>
> Signed-off-by: Alon Levy <address@hidden>
> ---
>  hw/qdev-properties.c |   60 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |   15 ++++++++++++
>  2 files changed, 75 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index a493087..3157721 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -63,6 +63,66 @@ PropertyInfo qdev_prop_bit = {
>      .print = print_bit,
>  };
>  
> +/* --- Enumeration --- */
> +/* Example usage:
> +EnumTable foo_enum_table[] = {
> +    {"bar", 1},
> +    {"buz", 2},
> +    {NULL, 0},
> +};
> +DEFINE_PROP_ENUM("foo", State, foo, 1, foo_enum_table),
> + */
> +static int parse_enum(DeviceState *dev, Property *prop, const char *str)
> +{
> +    uint8_t *ptr = qdev_get_prop_ptr(dev, prop);

uint8_t is inconsistent with print_enum() and DEFINE_PROP_ENUM(), which
both use uint32_t.

> +    EnumTable *option = (EnumTable*)prop->data;

Please don't cast from void * to pointer type (this isn't C++).

Not thrilled about the "void *data", to be honest.  Smells like
premature generality to me.

> +
> +    while (option->name != NULL) {
> +        if (!strncmp(str, option->name, strlen(option->name))) {

Why strncmp() and not straight strcmp()?

> +            *ptr = option->value;
> +            return 0;
> +        }
> +        option++;
> +    }
> +    return -EINVAL;
> +}
> +
> +static int print_enum(DeviceState *dev, Property *prop, char *dest, size_t 
> len)
> +{
> +    uint32_t *p = qdev_get_prop_ptr(dev, prop);
> +    EnumTable *option = (EnumTable*)prop->data;
> +    while (option->name != NULL) {
> +        if (*p == option->value) {
> +            return snprintf(dest, len, "%s", option->name);
> +        }
> +        option++;
> +    }
> +    return 0;

Bug: must dest[0] = 0 when returning 0.

> +}
> +
> +static int print_enum_options(DeviceInfo *info, Property *prop, char *dest, 
> size_t len)
> +{
> +    int ret = 0;
> +    EnumTable *option = (EnumTable*)prop->data;

Please don't cast from void * to pointer type (this isn't C++).

> +    while (option->name != NULL) {
> +        ret += snprintf(dest + ret, len - ret, "%s", option->name);
> +        if (option[1].name != NULL) {
> +            ret += snprintf(dest + ret, len - ret, "/");
> +        }
> +        option++;
> +    }
> +    return ret;
> +}
> +
> +PropertyInfo qdev_prop_enum = {
> +    .name  = "enum",
> +    .type  = PROP_TYPE_ENUM,
> +    .size  = sizeof(uint32_t),
> +    .parse = parse_enum,
> +    .print = print_enum,
> +    .print_options = print_enum_options,
> +};
> +
>  /* --- 8bit integer --- */
>  
>  static int parse_uint8(DeviceState *dev, Property *prop, const char *str)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 3d9acd7..3701d83 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -102,6 +102,7 @@ enum PropertyType {
>      PROP_TYPE_VLAN,
>      PROP_TYPE_PTR,
>      PROP_TYPE_BIT,
> +    PROP_TYPE_ENUM,
>  };
>  
>  struct PropertyInfo {
> @@ -121,6 +122,11 @@ typedef struct GlobalProperty {
>      QTAILQ_ENTRY(GlobalProperty) next;
>  } GlobalProperty;
>  
> +typedef struct EnumTable {
> +    const char *name;
> +    uint32_t    value;
> +} EnumTable;
> +
>  /*** Board API.  This should go away once we have a machine config file.  
> ***/
>  
>  DeviceState *qdev_create(BusState *bus, const char *name);
> @@ -235,6 +241,7 @@ extern PropertyInfo qdev_prop_drive;
>  extern PropertyInfo qdev_prop_netdev;
>  extern PropertyInfo qdev_prop_vlan;
>  extern PropertyInfo qdev_prop_pci_devfn;
> +extern PropertyInfo qdev_prop_enum;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -257,6 +264,14 @@ extern PropertyInfo qdev_prop_pci_devfn;
>              + type_check(uint32_t,typeof_field(_state, _field)), \
>          .defval    = (bool[]) { (_defval) },                     \
>          }
> +#define DEFINE_PROP_ENUM(_name, _state, _field, _defval, _options) {    \
> +        .name      = (_name),                                           \
> +        .info      = &(qdev_prop_enum),                                 \
> +        .offset    = offsetof(_state, _field)                           \
> +            + type_check(uint32_t,typeof_field(_state, _field)),        \
> +        .defval    = (uint32_t[]) { (_defval) },                        \
> +        .data      = (void*)(_options),                                 \

Please don't cast from pointer type to void * (this isn't C++).  If
someone accidentally passes an integral argument for _options (forgotten
operator &), the cast suppresses the warning.

> +        }
>  
>  #define DEFINE_PROP_UINT8(_n, _s, _f, _d)                       \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)

Okay, let's examine how your enumeration properties work.

An enumeration property describes a uint32_t field of the state object.
Differences to ordinary properties defined with DEFINE_PROP_UINT32:

* info is qdev_prop_enum instead of qdev_prop_uint32.  Differences
  between the two:

  - parse, print: symbolic names vs. numbers

  - name, print_options: only for -device DRIVER,\? (and name's use
    there isn't particularly helpful)

* data points to an EnumTable, which is a map string <-> number.  Thus,
  the actual enumeration is attached to the property declaration, not
  the property type (in programming languages, we commonly attach it to
  the type, not the variable declaration).  Since it's a table it can be
  used for multiple properties with minimal fuss.  Works for me.

What if we want to enumerate values of fields with types other than
uint32_t?

C enumeration types, in particular.  Tricky, because width and
signedness of enum types is implementation-defined, and different enum
types may differ there.

Perhaps what we really need is a way to define arbitrary integer type
properties with an EnumTable attached.