I haven't been able to follow the evolution of this series, my apologies if I'm missing things already discussed.
Alon Levy <al...@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 <al...@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.