Patchwork [03/16] qdev-properties: add PROP_TYPE_ENUM

login
register
mail settings
Submitter Alon Levy
Date Feb. 3, 2011, 10:45 p.m.
Message ID <1296773152-23279-4-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/81739/
State New
Headers show

Comments

Alon Levy - Feb. 3, 2011, 10:45 p.m.
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 <alevy@redhat.com>
---
 hw/qdev-properties.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |   15 ++++++++++++
 2 files changed, 75 insertions(+), 0 deletions(-)
Markus Armbruster - Feb. 7, 2011, 8:53 a.m.
I haven't been able to follow the evolution of this series, my apologies
if I'm missing things already discussed.

Alon Levy <alevy@redhat.com> 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 <alevy@redhat.com>
> ---
>  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.
Alon Levy - Feb. 7, 2011, 10:43 a.m.
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 <alevy@redhat.com> 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 <alevy@redhat.com>
> > ---
> >  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).
Anthony Liguori - Feb. 7, 2011, 1:15 p.m.
On 02/07/2011 04:43 AM, Alon Levy wrote:
> 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<alevy@redhat.com>  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<alevy@redhat.com>
>>> ---
>>>   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.
>    

This is wrong.

Libvirt should query this information through QMP.  This is my main 
concern with enums.  If there isn't a useful introspection interface, 
libvirt is going to do dumb things like query help output.

This has been a nightmare historically and I'm not inclined to repeat it.

Regards,

Anthony Liguori
Anthony Liguori - Feb. 7, 2011, 1:20 p.m.
On 02/07/2011 02:53 AM, 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<alevy@redhat.com>  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<alevy@redhat.com>
>> ---
>>   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.
>    

Given the massive QMP overhaul for 0.15, I don't want to add any more 
complexity to device properties because without unifying the device 
properties with QMP.

The bridge to QMP for qdev via device_add is quite scary.  We have 
strong typing in QMP, and then marshal those strong types to strings 
such that they can be demarshaled via device properties.  This is a real 
shame.

As I mentioned earlier, this patch breaks that in a subtle way.

If we want to add enums (and we really do), we should add it first to 
QMP in a structured way and then map that to device properties.

Regards,

Anthony Liguori

Regards,

Anthony Liguori
Markus Armbruster - Feb. 7, 2011, 2 p.m.
Alon Levy <alevy@redhat.com> writes:

> 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 <alevy@redhat.com> 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 <alevy@redhat.com>
>> > ---
>> >  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?

Doubt the additional complexity just for keeping EnumTable out of
Property is worth it.

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

Further down, we discuss the idea of supporting an EnumTable with
arbitrary integer type properties.  Would you find an EnumTable member
of Property more palatable then?

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

Print something useful is a good idea.  I guess I'd print an unadorned
"%d".

>> > +}
>> > +
[...]
>> > +        }
>> >  
>> >  #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.

Too terse, let me try again :)

   - name, print_options: differences not important here, because these
     members are they are only for -device DRIVER,\?

     By the way, I don't find help output like

        e1000.mac=macaddr
        e1000.vlan=vlan
        e1000.netdev=netdev

     particularly helpful.  Not your fault, outside the scope of this
     patch.

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

That's what I meant when I called it "tricky".

Still, having an enum property that cannot be used with enumeration
types is kind of sad, isn't it?

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

Sounds like what I have in mind.  Care to explore it?

One EnumTable should do, just make its member value wide enough.

Note to maintainer: we don't have to get enum properties 100% right and
polished before we can commit this series.
Markus Armbruster - Feb. 7, 2011, 2:05 p.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/07/2011 04:43 AM, Alon Levy wrote:
>> On Mon, Feb 07, 2011 at 09:53:44AM +0100, Markus Armbruster wrote:
[...]
>>> 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.
>>    
>
> This is wrong.
>
> Libvirt should query this information through QMP.

"We should make this information readily available through QMP", you
want to say ;)

>                                                     This is my main
> concern with enums.  If there isn't a useful introspection interface,
> libvirt is going to do dumb things like query help output.

Beggars can't be choosers.

> This has been a nightmare historically and I'm not inclined to repeat it.

No argument, but we can hardly expect poor Alon to finish the QMP job
just to get his usb-ccid device in.

What would making him drop enum properties from his series accomplish?
We'd get the same device with an inferior -device interface, which we
then have to maintain compatibly.
Alon Levy - Feb. 7, 2011, 2:14 p.m.
On Mon, Feb 07, 2011 at 07:15:57AM -0600, Anthony Liguori wrote:
> On 02/07/2011 04:43 AM, Alon Levy wrote:
> >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<alevy@redhat.com>  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<alevy@redhat.com>
> >>>---
> >>>  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.
> 
> This is wrong.
> 
> Libvirt should query this information through QMP.  This is my main
> concern with enums.  If there isn't a useful introspection
> interface, libvirt is going to do dumb things like query help
> output.
> 
> This has been a nightmare historically and I'm not inclined to repeat it.
> 

Fair enough, so a patch that added enumeration through QMP would be acceptable?
I'm not even sure that makes sense, would you mind outlining how you see this
implemented?

> Regards,
> 
> Anthony Liguori
> 
>
Anthony Liguori - Feb. 7, 2011, 2:21 p.m.
On 02/07/2011 08:05 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>    
>> On 02/07/2011 04:43 AM, Alon Levy wrote:
>>      
>>> On Mon, Feb 07, 2011 at 09:53:44AM +0100, Markus Armbruster wrote:
>>>        
> [...]
>    
>>>> 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.
>>>
>>>        
>> This is wrong.
>>
>> Libvirt should query this information through QMP.
>>      
> "We should make this information readily available through QMP", you
> want to say ;)
>
>    
>>                                                      This is my main
>> concern with enums.  If there isn't a useful introspection interface,
>> libvirt is going to do dumb things like query help output.
>>      
> Beggars can't be choosers.
>    

I'm making steady progress.  I just finished device_add/device_del on a 
plane.  I expect to have QMP fully converted in a couple weeks.

There is a lot more than I anticipated that is not available in QMP vs. 
the HMP so that's going to take a bit more time.


>> This has been a nightmare historically and I'm not inclined to repeat it.
>>      
> No argument, but we can hardly expect poor Alon to finish the QMP job
> just to get his usb-ccid device in.
>    

Hint: enum shouldn't block usb-ccid.  An enum is equivalent to a string 
property to convert as far as the current command line interface so just 
use a string property with a well documented set of inputs, and let's do 
enums properly.

That's what I was suggesting in my previous replies to this series.

> What would making him drop enum properties from his series accomplish?
>    

We (or maybe just I) have to take QMP more seriously.  I don't want to 
further make device properties incompatible with QMP which is what an 
enum property would do.

> We'd get the same device with an inferior -device interface, which we
> then have to maintain compatibly.
>    

We can retroactively make the command line interface take a symbolic 
value of an enum when it previously took a string with no compatibility 
problem.

But a QMP enum interface may not operate in strings.  If it sends 
integer constants instead of strings, then we'll create a compatibility 
issue.

Regards,

Anthony Liguori
Alon Levy - Feb. 7, 2011, 2:27 p.m.
On Mon, Feb 07, 2011 at 03:00:25PM +0100, Markus Armbruster wrote:
> Alon Levy <alevy@redhat.com> writes:
> 
> > 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 <alevy@redhat.com> 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 <alevy@redhat.com>
> >> > ---
> >> >  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?
> 
> Doubt the additional complexity just for keeping EnumTable out of
> Property is worth it.
> 
> > In summary I guess data is a terrible name, but it was least amount of change. Happy
> > to take suggestions.
> 
> Further down, we discuss the idea of supporting an EnumTable with
> arbitrary integer type properties.  Would you find an EnumTable member
> of Property more palatable then?
> 

I would.

> >> > +
> >> > +    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)
> 
> Print something useful is a good idea.  I guess I'd print an unadorned
> "%d".
> 

Agreed.

> >> > +}
> >> > +
> [...]
> >> > +        }
> >> >  
> >> >  #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.
> 
> Too terse, let me try again :)
> 
>    - name, print_options: differences not important here, because these
>      members are they are only for -device DRIVER,\?
> 
>      By the way, I don't find help output like
> 
>         e1000.mac=macaddr
>         e1000.vlan=vlan
>         e1000.netdev=netdev
> 
>      particularly helpful.  Not your fault, outside the scope of this
>      patch.
> 

Right, you get strange "X=X" output most of the time, but with print_options
for enum types you actually get this:

 $ x86_64-softmmu/qemu-system-x86_64 -device ccid-card-emulated,?
 ccid-card-emulated.backend=nss-emulated/certificates

That's actually parsable. Of course I agree with Anthony that having a json (QMP)
interface with properly quoted strings (here I don't take care of backslashes
in the EnumTable names for instance) would be much better.

> >> 
> >> * 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).
> 
> That's what I meant when I called it "tricky".
> 
> Still, having an enum property that cannot be used with enumeration
> types is kind of sad, isn't it?
> 
> >> 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).
> 
> Sounds like what I have in mind.  Care to explore it?
> 
> One EnumTable should do, just make its member value wide enough.
> 

Ok, I can try that. Sounds like it should work.

> Note to maintainer: we don't have to get enum properties 100% right and
> polished before we can commit this series.
>
Anthony Liguori - Feb. 7, 2011, 2:49 p.m.
On 02/07/2011 08:14 AM, Alon Levy wrote:
> Fair enough, so a patch that added enumeration through QMP would be acceptable?
> I'm not even sure that makes sense, would you mind outlining how you see this
> implemented?
>    

Before we write any code, we need to figure out what an enum is, how we 
want to transport it via QMP, and how we will support introspection on it.

In terms of the protocol format, I'm inclined to suggest that we 
transmit enums as integers as it maps better to C.  That way, we can 
write a first class enumeration type and build our interfaces on top of 
that.

I'm working on a QMP rewrite using a schema located at 
http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/glib

The schema has the notion of defining types along side with defining 
interfaces.  Adding a syntax to define an enum would be pretty straight 
forward.  Probably something like:

{ 'DriftCompensationPolicy': [{'gradual': 'int'}, {'fast': 'int'}, 
{'none': 'int'}] }

This would in turn generate:

typedef enum DriftCompensationPolicy {
     DRIFT_COMPENSATION_POLICY_GRADUAL = 0,
     DRIFT_COMPENSATION_POLICY_FAST,
     DRIFT_COMPENSATION_POLICY_NONE
} DriftCompensationPolicy;

 From a QMP side, the value would be marshalled as an integer but the 
schema would contain the type 'DriftCompensationPolicy' as the type.

For -device, this would mean that enums would be treated as integer 
properties, and we'd need some boiler plate code to register the enum 
with symbolic names.

 From a qdev perspective, I think it would be easier to generate a 
unique property type for every specific enum type.   That means the qdev 
side ends up looking like:

PropertyInfo qdev_prop_drift_compensation_policy = {
     .name  = "DriftCompensationPolicy",
     .type  = PROP_TYPE_INT32,
     .size  = sizeof(DriftCompensationPolicy),
     .parse = parse_drift_compensation_policy,
     .print = print_drift_compensation_policy,
};

struct MyDevice {
      DriftCompensationPolicy drift_policy;
};

DEFINE_PROP_DRIFT_COMPENSATION_POLICY("drift_policy", MyDevice, 
drift_policy,
                                                                                    DRIFT_COMPENSATION_POLICY_NONE);

We could autogenerate all of this code from the QMP schema too.  It's 
possible to do a one-off forwards compatible enum type for qdev but I'd 
strongly prefer to get the QMP infrastructure in place first.

But the advantage of this approach is pretty significant IMHO.  We get 
to work in native C enumeration types both in QEMU and libqmp.  That's a 
big win for type safety.

BTW, if we treat QMP introspection as just returning the QMP schema that 
we use for code generation (it's valid JSON afterall), then we get 
enumeration introspection for free.

I think this efficiently gives us what danpb's earlier series was going 
for with his introspection patch set.

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>>
>>      
>
Anthony Liguori - Feb. 7, 2011, 3:23 p.m.
On 02/07/2011 08:49 AM, Anthony Liguori wrote:
> On 02/07/2011 08:14 AM, Alon Levy wrote:
>> Fair enough, so a patch that added enumeration through QMP would be 
>> acceptable?
>> I'm not even sure that makes sense, would you mind outlining how you 
>> see this
>> implemented?
>
> Before we write any code, we need to figure out what an enum is, how 
> we want to transport it via QMP, and how we will support introspection 
> on it.
>
> In terms of the protocol format, I'm inclined to suggest that we 
> transmit enums as integers as it maps better to C.  That way, we can 
> write a first class enumeration type and build our interfaces on top 
> of that.
>
> I'm working on a QMP rewrite using a schema located at 
> http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/glib
>
> The schema has the notion of defining types along side with defining 
> interfaces.  Adding a syntax to define an enum would be pretty 
> straight forward.  Probably something like:
>
> { 'DriftCompensationPolicy': [{'gradual': 'int'}, {'fast': 'int'}, 
> {'none': 'int'}] }

http://repo.or.cz/w/qemu/aliguori.git/commit/50375b29f512c18d03cec8b1f9fea47ffdb6b232

Is a start at what I'm talking about here.  Adding the qdev type bits 
should be pretty straight forward.

Regards,

Anthony Liguori

>
> This would in turn generate:
>
> typedef enum DriftCompensationPolicy {
>     DRIFT_COMPENSATION_POLICY_GRADUAL = 0,
>     DRIFT_COMPENSATION_POLICY_FAST,
>     DRIFT_COMPENSATION_POLICY_NONE
> } DriftCompensationPolicy;
>
> From a QMP side, the value would be marshalled as an integer but the 
> schema would contain the type 'DriftCompensationPolicy' as the type.
>
> For -device, this would mean that enums would be treated as integer 
> properties, and we'd need some boiler plate code to register the enum 
> with symbolic names.
>
> From a qdev perspective, I think it would be easier to generate a 
> unique property type for every specific enum type.   That means the 
> qdev side ends up looking like:
>
> PropertyInfo qdev_prop_drift_compensation_policy = {
>     .name  = "DriftCompensationPolicy",
>     .type  = PROP_TYPE_INT32,
>     .size  = sizeof(DriftCompensationPolicy),
>     .parse = parse_drift_compensation_policy,
>     .print = print_drift_compensation_policy,
> };
>
> struct MyDevice {
>      DriftCompensationPolicy drift_policy;
> };
>
> DEFINE_PROP_DRIFT_COMPENSATION_POLICY("drift_policy", MyDevice, 
> drift_policy,
>                                                                                    
> DRIFT_COMPENSATION_POLICY_NONE);
>
> We could autogenerate all of this code from the QMP schema too.  It's 
> possible to do a one-off forwards compatible enum type for qdev but 
> I'd strongly prefer to get the QMP infrastructure in place first.
>
> But the advantage of this approach is pretty significant IMHO.  We get 
> to work in native C enumeration types both in QEMU and libqmp.  That's 
> a big win for type safety.
>
> BTW, if we treat QMP introspection as just returning the QMP schema 
> that we use for code generation (it's valid JSON afterall), then we 
> get enumeration introspection for free.
>
> I think this efficiently gives us what danpb's earlier series was 
> going for with his introspection patch set.
>
> Regards,
>
> Anthony Liguori
>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>
>
>
Alon Levy - Feb. 8, 2011, 3:34 p.m.
On Mon, Feb 07, 2011 at 04:27:21PM +0200, Alon Levy wrote:
> On Mon, Feb 07, 2011 at 03:00:25PM +0100, Markus Armbruster wrote:
> > Alon Levy <alevy@redhat.com> writes:
> > 
> > > 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 <alevy@redhat.com> 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 <alevy@redhat.com>
> > >> > ---
> > >> >  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?
> > 
> > Doubt the additional complexity just for keeping EnumTable out of
> > Property is worth it.
> > 
> > > In summary I guess data is a terrible name, but it was least amount of change. Happy
> > > to take suggestions.
> > 
> > Further down, we discuss the idea of supporting an EnumTable with
> > arbitrary integer type properties.  Would you find an EnumTable member
> > of Property more palatable then?
> > 
> 
> I would.
> 
> > >> > +
> > >> > +    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)
> > 
> > Print something useful is a good idea.  I guess I'd print an unadorned
> > "%d".
> > 
> 
> Agreed.
> 
> > >> > +}
> > >> > +
> > [...]
> > >> > +        }
> > >> >  
> > >> >  #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.
> > 
> > Too terse, let me try again :)
> > 
> >    - name, print_options: differences not important here, because these
> >      members are they are only for -device DRIVER,\?
> > 
> >      By the way, I don't find help output like
> > 
> >         e1000.mac=macaddr
> >         e1000.vlan=vlan
> >         e1000.netdev=netdev
> > 
> >      particularly helpful.  Not your fault, outside the scope of this
> >      patch.
> > 
> 
> Right, you get strange "X=X" output most of the time, but with print_options
> for enum types you actually get this:
> 
>  $ x86_64-softmmu/qemu-system-x86_64 -device ccid-card-emulated,?
>  ccid-card-emulated.backend=nss-emulated/certificates
> 
> That's actually parsable. Of course I agree with Anthony that having a json (QMP)
> interface with properly quoted strings (here I don't take care of backslashes
> in the EnumTable names for instance) would be much better.
> 
> > >> 
> > >> * 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).
> > 
> > That's what I meant when I called it "tricky".
> > 
> > Still, having an enum property that cannot be used with enumeration
> > types is kind of sad, isn't it?
> > 
> > >> 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).
> > 
> > Sounds like what I have in mind.  Care to explore it?
> > 
> > One EnumTable should do, just make its member value wide enough.
> > 
> 
> Ok, I can try that. Sounds like it should work.
> 

Changed my mind - Since libvirt is happy with a string, and this is not something
I really have time to pursue, and Anthony is making a better solution IIUC, I'm
dropping this.

> > Note to maintainer: we don't have to get enum properties 100% right and
> > polished before we can commit this series.
> > 
>
Markus Armbruster - Feb. 8, 2011, 3:47 p.m.
Alon Levy <alevy@redhat.com> writes:

> On Mon, Feb 07, 2011 at 04:27:21PM +0200, Alon Levy wrote:
>> On Mon, Feb 07, 2011 at 03:00:25PM +0100, Markus Armbruster wrote:
[...]
>> > Sounds like what I have in mind.  Care to explore it?
>> > 
>> > One EnumTable should do, just make its member value wide enough.
>> > 
>> 
>> Ok, I can try that. Sounds like it should work.
>> 
>
> Changed my mind - Since libvirt is happy with a string, and this is not something
> I really have time to pursue, and Anthony is making a better solution IIUC, I'm
> dropping this.

That's okay.

Patch

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);
+    EnumTable *option = (EnumTable*)prop->data;
+
+    while (option->name != NULL) {
+        if (!strncmp(str, option->name, strlen(option->name))) {
+            *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;
+}
+
+static int print_enum_options(DeviceInfo *info, Property *prop, char *dest, size_t len)
+{
+    int ret = 0;
+    EnumTable *option = (EnumTable*)prop->data;
+    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),                                 \
+        }
 
 #define DEFINE_PROP_UINT8(_n, _s, _f, _d)                       \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)