| Message ID | 1296773152-23279-4-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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).
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
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
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.
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.
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 > >
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
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. >
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 >> >> >> >
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 >>> >>> > >
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. > > >
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.
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)
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(-)