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

[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 <kraxel@redhat.com>
---
 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

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

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> 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.

Any good reason why you remove the list of propierties?

What I want to do, to make usb modular I need two things:

a- a way to defino an alias, that "mouse" is equivalent to "QEMU USB
   Keyboard".  One is the qdev name and the other is the name passed to
   --usbdevice name.

b- things like disk are composed of:
   "disk" : "rest of disk arguments"

   My plan was to add both of them as properties or similar.  Any better
   idea?  I can put it in one struct or similar, but it is not trivial
   to find where to put things really, because the "mouse" part is
   filled in usb-hid.c or equivalent, and the "disk" arguments part is
   filled in vl.c

Later, Juan.

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

[Paul Brook]

> What I want to do, to make usb modular I need two things:
>
> a- a way to defino an alias, that "mouse" is equivalent to "QEMU USB
>    Keyboard".  One is the qdev name and the other is the name passed to
>    --usbdevice name.

I'm not too bothered about this. We're changing the option, so I don't see any 
particular problem with changing the device names at the same time.

> b- things like disk are composed of:
>    "disk" : "rest of disk arguments"
>
>    My plan was to add both of them as properties or similar.  Any better
>    idea?  I can put it in one struct or similar, but it is not trivial
>    to find where to put things really, because the "mouse" part is
>    filled in usb-hid.c or equivalent, and the "disk" arguments part is
>    filled in vl.c

I think this is confusing host configuration with machine configuration. This 
has been discussed before.

Paul

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

[Gerd Hoffmann]

On 06/30/09 16:03, Juan Quintela wrote:
> Gerd Hoffmann<kraxel@redhat.com>  wrote:
>> 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.
>
> Any good reason why you remove the list of propierties?

Pointless indirection and extra storage.
Note that this doesn't mean properties itself go away.

> a- a way to defino an alias, that "mouse" is equivalent to "QEMU USB
>     Keyboard".  One is the qdev name and the other is the name passed to
>     --usbdevice name.

stick an alias next to name in DeviceInfo?
I don't see why this should use properties.

> b- things like disk are composed of:
>     "disk" : "rest of disk arguments"

No.  You'll create the host state from most of "rest of disk arguments". 
  You'll attach a name property to qdev so the driver has a handle to 
lookup the host side of things.

cheers,
   Gerd

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

[Juan Quintela]

Paul Brook <paul@codesourcery.com> wrote:
>> What I want to do, to make usb modular I need two things:
>>
>> a- a way to defino an alias, that "mouse" is equivalent to "QEMU USB
>>    Keyboard".  One is the qdev name and the other is the name passed to
>>    --usbdevice name.
>
> I'm not too bothered about this. We're changing the option, so I don't see any 
> particular problem with changing the device names at the same time.

backward compatibility?  Not that I care about this one really.

>> b- things like disk are composed of:
>>    "disk" : "rest of disk arguments"
>>
>>    My plan was to add both of them as properties or similar.  Any better
>>    idea?  I can put it in one struct or similar, but it is not trivial
>>    to find where to put things really, because the "mouse" part is
>>    filled in usb-hid.c or equivalent, and the "disk" arguments part is
>>    filled in vl.c
>
> I think this is confusing host configuration with machine configuration. This 
> has been discussed before.

Yes, but it is the same problem. We are going to need something like:

-usb-hardawre name=bar,... -usbdevice disk:name=bar

or anything like that, the thing that I mean is that disk is going to
have always a parameter (what disk we mean) and mouse is not going
(necesarely)

Later, Juan.

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

[Paul Brook]

On Tuesday 30 June 2009, Juan Quintela wrote:
> Paul Brook <paul@codesourcery.com> wrote:
> >> What I want to do, to make usb modular I need two things:
> >>
> >> a- a way to defino an alias, that "mouse" is equivalent to "QEMU USB
> >>    Keyboard".  One is the qdev name and the other is the name passed to
> >>    --usbdevice name.
> >
> > I'm not too bothered about this. We're changing the option, so I don't
> > see any particular problem with changing the device names at the same
> > time.
>
> backward compatibility?  Not that I care about this one really.

This is a new option. There's nothing to be backwards compatible with.

> >> b- things like disk are composed of:
> >>    "disk" : "rest of disk arguments"
> >>
> > I think this is confusing host configuration with machine configuration.
> > This has been discussed before.
>
> Yes, but it is the same problem. We are going to need something like:
>
> -usb-hardawre name=bar,... -usbdevice disk:name=bar
>
> or anything like that, the thing that I mean is that disk is going to
> have always a parameter (what disk we mean) and mouse is not going
> (necesarely)

I'm not sure we're discussing the same thing here. "rest of disk arguments" 
should not be part of the USB device. The USB device should just have a link 
to a drive object, which is configured elsewhere (e.g. via a -drive option).

Paul