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

[Alon Levy]

On Mon, Feb 07, 2011 at 09:53:44AM +0100, Markus Armbruster wrote:
> 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.

Thanks, fixing.

> 
> > +    EnumTable *option = (EnumTable*)prop->data;
> 
> Please don't cast from void * to pointer type (this isn't C++).
> 

Will fix for all references.

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

I did put it in there because I didn't think a "EnumTable *enum" variable
would have been acceptable (added baggage just used by a single property type),
and I didn't find any other place to add it. I guess I should just do a:

typedef struct EnumProperty {
    Property base;
    EnumTable *table;
} EnumProperty;

But then because we define the properties in a Property[] array this won't work.
Maybe turn that into a Property* array?

In summary I guess data is a terrible name, but it was least amount of change. 
Happy
to take suggestions.

> > +
> > +    while (option->name != NULL) {
> > +        if (!strncmp(str, option->name, strlen(option->name))) {
> 
> Why strncmp() and not straight strcmp()?
> 

I guess no reason except "strncmp is more secure" but irrelevant here since
option->name is from the source, I'll fix.

> > +            *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.
> 

will just return snprintf(dest, len, "<enum %d>", option->value)

> > +}
> > +
> > +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++).
> 

fixing.

> > +    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.
> 

fixing.

> > +        }
> >  
> >  #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)

Why do you say that? this is being used by libvirt to get the names of the
supported backends for the ccid-card-emulated device.

> 
> * 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.
> 

I specifically used uint32_t expecting it to work for many uses. It would
be better to allow an arbitrary type, or just not require specifying the
type but getting it from the enum itself (using typeof?). But then you
can't have a single EnumTable because it's type is now dependent on the
enumeration type (of course I could resort to macros, but I don't want to
go there).

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

This sounds more promising. So you would have an EnumTableU32 etc and
DEFINE_PROP_{UINT8,..}_ENUM which takes an extra EnumTable of the correct
type, to be defined inline maybe so user doesn't actually declare it (like
no one declares Property thanks to the DEFINE_PROP_ macros).