From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52731 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PmR0x-0007fz-J7 for qemu-devel@nongnu.org; Mon, 07 Feb 2011 08:25:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PmR0l-00045j-R5 for qemu-devel@nongnu.org; Mon, 07 Feb 2011 08:20:12 -0500 Received: from mail-bw0-f45.google.com ([209.85.214.45]:35695) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PmR0l-00045Z-Hu for qemu-devel@nongnu.org; Mon, 07 Feb 2011 08:20:11 -0500 Received: by bwz16 with SMTP id 16so5004576bwz.4 for ; Mon, 07 Feb 2011 05:20:10 -0800 (PST) Message-ID: <4D4FF185.4090707@codemonkey.ws> Date: Mon, 07 Feb 2011 07:20:05 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 03/16] qdev-properties: add PROP_TYPE_ENUM References: <1296773152-23279-1-git-send-email-alevy@redhat.com> <1296773152-23279-4-git-send-email-alevy@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Alon Levy , qemu-devel@nongnu.org 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 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 >> --- >> 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