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

[Anthony Liguori <anthony@codemonkey.ws>]

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