[Qemu-devel] [RfC PATCH] qdev: rework device properties.

[Gerd Hoffmann]

Early RfC patch.  Not functional yet.  Just for comments right now.

Overall plan:
 * drop property lists. The properties are saved directly in the
   device state structs instead.
 * drop qdev_get_prop* functions, not needed any more.
 * replace qdev_set_prop* functions by qdev_prop_{parse,set*}.

Done:
 * added code to handle properties.

Todo:
 * convert all the device drivers.
 * actually drop the old functions.

Signed-off-by: Gerd Hoffmann <address@hidden>
---
 Makefile             |    2 +-
 hw/qdev-properties.c |  166 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.c            |   80 ++----------------------
 hw/qdev.h            |   44 +++++++++-----
 hw/syborg_timer.c    |   10 ++-
 5 files changed, 208 insertions(+), 94 deletions(-)
 create mode 100644 hw/qdev-properties.c

diff --git a/Makefile b/Makefile
index 918e927..af5b190 100644
--- a/Makefile
+++ b/Makefile
@@ -108,7 +108,7 @@ obj-y += bt-hci-csr.o
 obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o
 obj-y += qemu-char.o aio.o net-checksum.o savevm.o cache-utils.o
 obj-y += msmouse.o ps2.o
-obj-y += qdev.o ssi.o
+obj-y += qdev.o qdev-properties.o ssi.o
 
 obj-$(CONFIG_BRLAPI) += baum.o
 
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
new file mode 100644
index 0000000..fc8cb9f
--- /dev/null
+++ b/hw/qdev-properties.c
@@ -0,0 +1,166 @@
+#include "qdev.h"
+
+static void *qdev_prop_ptr(DeviceState *dev, Property *prop)
+{
+    void *ptr = dev;
+    ptr += prop->offset;
+    return ptr;
+}
+
+/* --- 32bit integer --- */
+
+static int parse_uint32(DeviceState *dev, Property *prop, const char *str)
+{
+    uint32_t *ptr = qdev_prop_ptr(dev, prop);
+
+    if (sscanf(str, "%" PRIu32, ptr) != 1)
+        return -1;
+    return 0;
+}
+
+static int print_uint32(DeviceState *dev, Property *prop, char *dest, size_t 
len)
+{
+    uint32_t *ptr = qdev_prop_ptr(dev, prop);
+    return snprintf(dest, len, "%" PRIu32, *ptr);
+}
+
+PropertyInfo qdev_prop_uint32 = {
+    .name  = "uint32",
+    .size  = sizeof(uint32_t),
+    .parse = parse_uint32,
+    .print = print_uint32,
+};
+
+/* --- 32bit hex value --- */
+
+static int parse_hex32(DeviceState *dev, Property *prop, const char *str)
+{
+    uint32_t *ptr = qdev_prop_ptr(dev, prop);
+
+    if (sscanf(str, "%" PRIx32, ptr) != 1)
+        return -1;
+    return 0;
+}
+
+static int print_hex32(DeviceState *dev, Property *prop, char *dest, size_t 
len)
+{
+    uint32_t *ptr = qdev_prop_ptr(dev, prop);
+    return snprintf(dest, len, "0x%" PRIx32, *ptr);
+}
+
+PropertyInfo qdev_prop_hex32 = {
+    .name  = "hex32",
+    .size  = sizeof(uint32_t),
+    .parse = parse_hex32,
+    .print = print_hex32,
+};
+
+/* --- mac address --- */
+
+/*
+ * accepted syntax versions:
+ *   01:02:03:04:05:06
+ *   01-02-03-04-05-06
+ */
+static int parse_mac(DeviceState *dev, Property *prop, const char *str)
+{
+    uint8_t *mac = qdev_prop_ptr(dev, prop);
+    int i, pos;
+    char *p;
+
+    for (i = 0, pos = 0; i < 6; i++, pos += 3) {
+        if (!isxdigit(str[pos]))
+            return -1;
+        if (!isxdigit(str[pos+1]))
+            return -1;
+        if (i == 5 && str[pos+2] != '\0')
+            return -1;
+        if (str[pos+2] != ':' && str[pos+2] != '-')
+            return -1;
+        mac[i] = strtol(str+pos, &p, 16);
+    }
+    return 0;
+}
+
+static int print_mac(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    uint8_t *mac = qdev_prop_ptr(dev, prop);
+    return snprintf(dest, len, "%02x:%02x:%02x:%02x:%02x:%02x",
+                    mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+}
+
+PropertyInfo qdev_prop_mac = {
+    .name  = "mac",
+    .size  = 6,
+    .parse = parse_mac,
+    .print = print_mac,
+};
+
+/* --- public helpers --- */
+
+static Property *qdev_prop_find(DeviceState *dev, const char *name)
+{
+    Property *props;
+
+    /* device properties */
+    props = dev->info->props;
+    while (props->name) {
+        if (strcmp(props->name, name) == 0)
+            return props;
+        props++;
+    }
+
+    /* bus properties */
+    props = dev->info->props;
+    while (props->name) {
+        if (strcmp(props->name, name) == 0)
+            return props;
+        props++;
+    }
+
+    return NULL;
+}
+
+int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
+{
+    Property *prop;
+
+    prop = qdev_prop_find(dev, name);
+    if (!prop) {
+        fprintf(stderr, "property \"%s.%s\" not found\n",
+                dev->info->name, name);
+        return -1;
+    }
+    if (!prop->info->parse) {
+        fprintf(stderr, "property \"%s.%s\" has no parser\n",
+                dev->info->name, name);
+        return -1;
+    }
+    return prop->info->parse(dev, prop, value);
+}
+
+int qdev_prop_set(DeviceState *dev, const char *name, void *src, size_t size)
+{
+    Property *prop;
+    void *dst;
+
+    prop = qdev_prop_find(dev, name);
+    if (!prop) {
+        fprintf(stderr, "property \"%s.%s\" not found\n",
+                dev->info->name, name);
+        return -1;
+    }
+    if (prop->info->size != size) {
+        fprintf(stderr, "property \"%s.%s\" size mismatch (%zd / %zd)\n",
+                dev->info->name, name, prop->info->size, size);
+        return -1;
+    }
+    dst = qdev_prop_ptr(dev, prop);
+    memcpy(dst, src, size);
+    return 0;
+}
+
+int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
+{
+    return qdev_prop_set(dev, name, &value, sizeof(value));
+}
diff --git a/hw/qdev.c b/hw/qdev.c
index 8d19cbe..ab23cd2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -31,16 +31,6 @@
 #include "sysemu.h"
 #include "monitor.h"
 
-struct DeviceProperty {
-    const char *name;
-    DevicePropType type;
-    union {
-        uint64_t i;
-        void *ptr;
-    } value;
-    DeviceProperty *next;
-};
-
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
 extern struct BusInfo system_bus_info;
@@ -112,52 +102,22 @@ void qdev_free(DeviceState *dev)
     free(dev);
 }
 
-static DeviceProperty *create_prop(DeviceState *dev, const char *name,
-                                   DevicePropType type)
-{
-    DeviceProperty *prop;
-
-    /* TODO: Check for duplicate properties.  */
-    prop = qemu_mallocz(sizeof(*prop));
-    prop->name = qemu_strdup(name);
-    prop->type = type;
-    prop->next = dev->props;
-    dev->props = prop;
-
-    return prop;
-}
-
 void qdev_set_prop_int(DeviceState *dev, const char *name, uint64_t value)
 {
-    DeviceProperty *prop;
-
-    prop = create_prop(dev, name, PROP_TYPE_INT);
-    prop->value.i = value;
 }
 
 void qdev_set_prop_dev(DeviceState *dev, const char *name, DeviceState *value)
 {
-    DeviceProperty *prop;
-
-    prop = create_prop(dev, name, PROP_TYPE_DEV);
-    prop->value.ptr = value;
 }
 
 void qdev_set_prop_ptr(DeviceState *dev, const char *name, void *value)
 {
-    DeviceProperty *prop;
-
-    prop = create_prop(dev, name, PROP_TYPE_PTR);
-    prop->value.ptr = value;
 }
 
 void qdev_set_netdev(DeviceState *dev, NICInfo *nd)
 {
-    assert(!dev->nd);
-    dev->nd = nd;
 }
 
-
 /* Get a character (serial) device interface.  */
 CharDriverState *qdev_init_chardev(DeviceState *dev)
 {
@@ -176,50 +136,19 @@ BusState *qdev_get_parent_bus(DeviceState *dev)
     return dev->parent_bus;
 }
 
-static DeviceProperty *find_prop(DeviceState *dev, const char *name,
-                                 DevicePropType type)
-{
-    DeviceProperty *prop;
-
-    for (prop = dev->props; prop; prop = prop->next) {
-        if (strcmp(prop->name, name) == 0) {
-            assert (prop->type == type);
-            return prop;
-        }
-    }
-    return NULL;
-}
-
 uint64_t qdev_get_prop_int(DeviceState *dev, const char *name, uint64_t def)
 {
-    DeviceProperty *prop;
-
-    prop = find_prop(dev, name, PROP_TYPE_INT);
-    if (!prop) {
-        return def;
-    }
-
-    return prop->value.i;
+    return def;
 }
 
 void *qdev_get_prop_ptr(DeviceState *dev, const char *name)
 {
-    DeviceProperty *prop;
-
-    prop = find_prop(dev, name, PROP_TYPE_PTR);
-    assert(prop);
-    return prop->value.ptr;
+    return NULL;
 }
 
 DeviceState *qdev_get_prop_dev(DeviceState *dev, const char *name)
 {
-    DeviceProperty *prop;
-
-    prop = find_prop(dev, name, PROP_TYPE_DEV);
-    if (!prop) {
-        return NULL;
-    }
-    return prop->value.ptr;
+    return NULL;
 }
 
 void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
@@ -345,7 +274,6 @@ static void qbus_print(Monitor *mon, BusState *bus, int 
indent);
 
 static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
-    DeviceProperty *prop;
     BusState *child;
     qdev_printf("dev: %s\n", dev->info->name);
     indent += 2;
@@ -355,6 +283,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int 
indent)
     if (dev->num_gpio_out) {
         qdev_printf("gpio-out %d\n", dev->num_gpio_out);
     }
+#if 0
     for (prop = dev->props; prop; prop = prop->next) {
         switch (prop->type) {
         case PROP_TYPE_INT:
@@ -373,6 +302,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int 
indent)
             break;
         }
     }
+#endif
     if (dev->parent_bus->info->print)
         dev->parent_bus->info->print(mon, dev, indent);
     LIST_FOREACH(child, &dev->child_bus, sibling) {
diff --git a/hw/qdev.h b/hw/qdev.h
index 2dbf414..c7a8d18 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -4,9 +4,11 @@
 #include "hw.h"
 #include "sys-queue.h"
 
-typedef struct DeviceInfo DeviceInfo;
+typedef struct Property Property;
+
+typedef struct PropertyInfo PropertyInfo;
 
-typedef struct DeviceProperty DeviceProperty;
+typedef struct DeviceInfo DeviceInfo;
 
 typedef struct BusState BusState;
 
@@ -17,7 +19,6 @@ typedef struct BusInfo BusInfo;
 struct DeviceState {
     DeviceInfo *info;
     BusState *parent_bus;
-    DeviceProperty *props;
     int num_gpio_out;
     qemu_irq *gpio_out;
     int num_gpio_in;
@@ -31,6 +32,7 @@ typedef void (*bus_dev_print)(Monitor *mon, DeviceState *dev, 
int indent);
 struct BusInfo {
     const char *name;
     size_t size;
+    Property *props;
     bus_dev_print print;
     int next_busnr;
 };
@@ -45,6 +47,19 @@ struct BusState {
     TAILQ_ENTRY(BusState) next;
 };
 
+struct Property {
+    const char   *name;
+    PropertyInfo *info;
+    int          offset;
+};
+
+struct PropertyInfo {
+    const char *name;
+    size_t size;
+    int (*parse)(DeviceState *dev, Property *prop, const char *str);
+    int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
+};
+
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 DeviceState *qdev_create(BusState *bus, const char *name);
@@ -64,23 +79,12 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char 
*name);
 
 /*** Device API.  ***/
 
-typedef enum {
-    PROP_TYPE_INT,
-    PROP_TYPE_PTR,
-    PROP_TYPE_DEV
-} DevicePropType;
-
-typedef struct {
-    const char *name;
-    DevicePropType type;
-} DevicePropList;
-
 typedef void (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
 
 struct DeviceInfo {
     const char *name;
     size_t size;
-    DevicePropList *props;
+    Property *props;
 
     /* Private to qdev / bus.  */
     qdev_initfn init;
@@ -129,4 +133,14 @@ void do_info_qdrv(Monitor *mon);
 /* helper for "-${bus}device ?" */
 void do_list_qdrv(BusInfo *bus);
 
+/*** qdev-properties.c ***/
+
+extern PropertyInfo qdev_prop_uint32;
+extern PropertyInfo qdev_prop_hex32;
+extern PropertyInfo qdev_prop_mac;
+
+int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
+int qdev_prop_set(DeviceState *dev, const char *name, void *src, size_t size);
+int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
+
 #endif
diff --git a/hw/syborg_timer.c b/hw/syborg_timer.c
index 4f5e3a1..f3d00d8 100644
--- a/hw/syborg_timer.c
+++ b/hw/syborg_timer.c
@@ -230,9 +230,13 @@ static SysBusDeviceInfo syborg_timer_info = {
     .init = syborg_timer_init,
     .qdev.name  = "syborg,timer",
     .qdev.size  = sizeof(SyborgTimerState),
-    .qdev.props = (DevicePropList[]) {
-        {.name = "frequency", .type = PROP_TYPE_INT},
-        {.name = NULL}
+    .qdev.props = (Property[]) {
+        {
+            .name   = "frequency",
+            .info   = &qdev_prop_uint32,
+            .offset = offsetof(SyborgTimerState, freq),
+        },
+        {/* end of list */}
     }
 };
 
-- 
1.6.2.5