[Qemu-devel] [RfC PATCH 0/2] qdev/prop: type checking.

[Qemu-devel] [RfC PATCH 0/2] qdev/prop: type checking.

[Gerd Hoffmann]

  Hi,

Two patches to improve qdev attribute type checking.  Goes on top of the
qdev bits in anthonys patch queue and the other qdev/prop patches posted
today.

The first adds a type field to PropertyInfo and switches the
qdev_prop_set_*() functions from size checkinf to type checking.  The
functions are also changed to not return an error and abort() instead.

The second helps a bunch of helper macros to help creating property
declarations and converts pci.c as example.  I'm not that happy with
that one yet.  Especially I'd like to check somehow that
typeof(_state->_field) == _type.  But couldn't figure out a way to do
so.  As we are setting up static data structures we are quite limited in
what we can do here.  The typechecking trick used by the linux kernel
min/max macros can't be used for example.

comments & ideas are welcome,

  Gerd

[Qemu-devel] [PATCH 1/2] qdev/prop: add property type.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev-addr.c       |    5 +++--
 hw/qdev-addr.h       |    2 +-
 hw/qdev-properties.c |   36 ++++++++++++++++++++----------------
 hw/qdev.h            |   18 ++++++++++++++----
 4 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index f1bf2fa..305c2d3 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -20,12 +20,13 @@ static int print_taddr(DeviceState *dev, Property *prop, char *dest, size_t len)
 
 PropertyInfo qdev_prop_taddr = {
     .name  = "taddr",
+    .type  = PROP_TYPE_TADDR,
     .size  = sizeof(target_phys_addr_t),
     .parse = parse_taddr,
     .print = print_taddr,
 };
 
-int qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
+void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
 {
-    return qdev_prop_set(dev, name, &value, sizeof(value));
+    qdev_prop_set(dev, name, &value, PROP_TYPE_TADDR);
 }
diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
index 389c1d6..f02bd7a 100644
--- a/hw/qdev-addr.h
+++ b/hw/qdev-addr.h
@@ -1,2 +1,2 @@
 extern PropertyInfo qdev_prop_taddr;
-int qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value);
+void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value);
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 3c31e31..ea937ae 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -29,6 +29,7 @@ static int print_uint16(DeviceState *dev, Property *prop, char *dest, size_t len
 
 PropertyInfo qdev_prop_uint16 = {
     .name  = "uint16",
+    .type  = PROP_TYPE_UINT16,
     .size  = sizeof(uint16_t),
     .parse = parse_uint16,
     .print = print_uint16,
@@ -56,6 +57,7 @@ static int print_uint32(DeviceState *dev, Property *prop, char *dest, size_t len
 
 PropertyInfo qdev_prop_uint32 = {
     .name  = "uint32",
+    .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_uint32,
     .print = print_uint32,
@@ -80,6 +82,7 @@ static int print_hex32(DeviceState *dev, Property *prop, char *dest, size_t len)
 
 PropertyInfo qdev_prop_hex32 = {
     .name  = "hex32",
+    .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_hex32,
     .print = print_hex32,
@@ -95,6 +98,7 @@ static int print_ptr(DeviceState *dev, Property *prop, char *dest, size_t len)
 
 PropertyInfo qdev_prop_ptr = {
     .name  = "ptr",
+    .type  = PROP_TYPE_PTR,
     .size  = sizeof(void*),
     .print = print_ptr,
 };
@@ -135,6 +139,7 @@ static int print_mac(DeviceState *dev, Property *prop, char *dest, size_t len)
 
 PropertyInfo qdev_prop_mac = {
     .name  = "mac",
+    .type  = PROP_TYPE_MACADDR,
     .size  = 6,
     .parse = parse_mac,
     .print = print_mac,
@@ -189,40 +194,39 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     return prop->info->parse(dev, prop, value);
 }
 
-int qdev_prop_set(DeviceState *dev, const char *name, void *src, size_t size)
+void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type)
 {
     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;
+        fprintf(stderr, "%s: property \"%s.%s\" not found\n",
+                __FUNCTION__, dev->info->name, name);
+        abort();
     }
-    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;
+    if (prop->info->type != type) {
+        fprintf(stderr, "%s: property \"%s.%s\" type mismatch\n",
+                __FUNCTION__, dev->info->name, name);
+        abort();
     }
     dst = qdev_get_prop_ptr(dev, prop);
-    memcpy(dst, src, size);
-    return 0;
+    memcpy(dst, src, prop->info->size);
 }
 
-int qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
+void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
 {
-    return qdev_prop_set(dev, name, &value, sizeof(value));
+    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT16);
 }
 
-int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
+void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
 {
-    return qdev_prop_set(dev, name, &value, sizeof(value));
+    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT32);
 }
 
-int qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
+void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 {
-    return qdev_prop_set(dev, name, &value, sizeof(value));
+    qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
 }
 
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
diff --git a/hw/qdev.h b/hw/qdev.h
index 6b35961..584617e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -54,9 +54,19 @@ struct Property {
     void         *defval;
 };
 
+enum PropertyType {
+    PROP_TYPE_UNSPEC = 0,
+    PROP_TYPE_UINT16,
+    PROP_TYPE_UINT32,
+    PROP_TYPE_TADDR,
+    PROP_TYPE_MACADDR,
+    PROP_TYPE_PTR,
+};
+
 struct PropertyInfo {
     const char *name;
     size_t size;
+    enum PropertyType type;
     int (*parse)(DeviceState *dev, Property *prop, const char *str);
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
 };
@@ -137,11 +147,11 @@ extern PropertyInfo qdev_prop_mac;
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
 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_uint16(DeviceState *dev, const char *name, uint16_t value);
-int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
+void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type);
+void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
+void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
 /* FIXME: Remove opaque pointer properties.  */
-int qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
+void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 #endif
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/2] qdev/prop: helper macros.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c       |    5 +++++
 hw/qdev-addr.h |    3 +++
 hw/qdev.h      |   20 ++++++++++++++++++++
 3 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 4e92c4c..1a1cbc6 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -56,6 +56,7 @@ static struct BusInfo pci_bus_info = {
     .print_dev  = pcibus_dev_print,
     .add_dev    = pci_create,
     .props      = (Property[]) {
+#if 0
         {
             .name   = "devfn",
             .info   = &qdev_prop_uint32,
@@ -63,6 +64,10 @@ static struct BusInfo pci_bus_info = {
             .defval = (uint32_t[]) { -1 },
         },
         {/* end of list */}
+#else
+        DEF_PROP_UINT32("devfn", PCIDevice, devfn, -1),
+        DEF_PROP_END_OF_LIST()
+#endif
     }
 };
 
diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
index f02bd7a..7160989 100644
--- a/hw/qdev-addr.h
+++ b/hw/qdev-addr.h
@@ -1,2 +1,5 @@
+#define DEF_PROP_TADDR(_n, _s, _f, _d)                                  \
+    DEF_PROP(_n, _s, _f, _d, target_phys_addr_t, qdev_prop_taddr)
+
 extern PropertyInfo qdev_prop_taddr;
 void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value);
diff --git a/hw/qdev.h b/hw/qdev.h
index 584617e..1edefcc 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -144,6 +144,26 @@ extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_mac;
 
+#define DEF_PROP(_name, _state, _field, _defval, _type, _prop) { \
+        .name      = (_name),                                    \
+        .info      = &(_prop),                                   \
+        .offset    = offsetof(_state, _field),                   \
+        .defval    = (_type[]) { (_defval) },                    \
+        }
+
+#define DEF_PROP_UINT16(_n, _s, _f, _d)                         \
+    DEF_PROP(_n, _s, _f, _d, uint16_t, qdev_prop_uint16)
+#define DEF_PROP_UINT32(_n, _s, _f, _d)                         \
+    DEF_PROP(_n, _s, _f, _d, uint32_t, qdev_prop_uint32)
+#define DEF_PROP_HEX32(_n, _s, _f, _d)                  \
+    DEF_PROP(_n, _s, _f, _d, uint32_t, qdev_prop_hex32)
+#define DEF_PROP_PTR(_n, _s, _f, _d)                    \
+    DEF_PROP(_n, _s, _f, _d, void*, qdev_prop_ptr)
+#define DEF_PROP_MAC(_n, _s, _f, _d)                    \
+    DEF_PROP(_n, _s, _f, _d, void*, qdev_prop_mac)
+#define DEF_PROP_END_OF_LIST()                  \
+    {}
+
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
-- 
1.6.2.5

Re: [Qemu-devel] [RfC PATCH 0/2] qdev/prop: type checking.

[Paul Brook]

> The second helps a bunch of helper macros to help creating property
> declarations and converts pci.c as example.  I'm not that happy with
> that one yet.  Especially I'd like to check somehow that
> typeof(_state->_field) == _type.  But couldn't figure out a way to do
> so.  As we are setting up static data structures we are quite limited in
> what we can do here.  The typechecking trick used by the linux kernel
> min/max macros can't be used for example.

Could we use a union rather than an opaque pointer for the default value?

Paul

Re: [Qemu-devel] [RfC PATCH 0/2] qdev/prop: type checking.

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

On 07/13/09 17:36, Paul Brook wrote:
>> The second helps a bunch of helper macros to help creating property
>> declarations and converts pci.c as example.  I'm not that happy with
>> that one yet.  Especially I'd like to check somehow that
>> typeof(_state->_field) == _type.  But couldn't figure out a way to do
>> so.  As we are setting up static data structures we are quite limited in
>> what we can do here.  The typechecking trick used by the linux kernel
>> min/max macros can't be used for example.
>
> Could we use a union rather than an opaque pointer for the default value?

See patch (not functional, just to show the macros).

Improves things only a little bit.  We still don't have any type 
checking for the struct element we are writing to.  We have the field 
offset and the type it is supposed to be.  But no actual check.

cheers,
   Gerd

[-- Attachment #2: 0001-qdev-prop-helper-macros.patch --]
[-- Type: text/plain, Size: 3401 bytes --]

>From a770efe5a41de0bc841e935bf0a102ba643db031 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Mon, 13 Jul 2009 14:00:18 +0200
Subject: [PATCH] qdev/prop: helper macros.

---
 hw/pci.c       |    5 +++++
 hw/qdev-addr.h |    3 +++
 hw/qdev.h      |   32 +++++++++++++++++++++++++++++++-
 3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 4c61d63..e689d35 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -56,6 +56,7 @@ static struct BusInfo pci_bus_info = {
     .print_dev  = pcibus_dev_print,
     .add_dev    = pci_create,
     .props      = (Property[]) {
+#if 0
         {
             .name   = "devfn",
             .info   = &qdev_prop_uint32,
@@ -63,6 +64,10 @@ static struct BusInfo pci_bus_info = {
             .defval = (uint32_t[]) { -1 },
         },
         {/* end of list */}
+#else
+        DEFINE_PROP_UINT32("devfn", PCIDevice, devfn, -1),
+        DEFINE_PROP_END_OF_LIST()
+#endif
     }
 };
 
diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
index f02bd7a..e135a96 100644
--- a/hw/qdev-addr.h
+++ b/hw/qdev-addr.h
@@ -1,2 +1,5 @@
+#define DEFINE_PROP_TADDR(_n, _s, _f)           \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_taddr)
+
 extern PropertyInfo qdev_prop_taddr;
 void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value);
diff --git a/hw/qdev.h b/hw/qdev.h
index ed0ae83..e7f2367 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -51,7 +51,12 @@ struct Property {
     const char   *name;
     PropertyInfo *info;
     int          offset;
-    void         *defval;
+    void         *defval; /* FIXME: delete */
+    union {
+        uint16_t *uint16_t;
+        uint32_t *uint32_t;
+        void     *any;
+    } def;
 };
 
 enum PropertyType {
@@ -144,6 +149,31 @@ extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_mac;
 
+#define DEFINE_PROP(_name, _state, _field, _prop) { \
+        .name      = (_name),                                    \
+        .info      = &(_prop),                                   \
+        .offset    = offsetof(_state, _field),                   \
+        }
+#define DEFINE_PROP_DEFAULT(_name, _state, _field, _defval, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field),                          \
+        .def._type = (_type[]) { _defval },                             \
+        }
+
+#define DEFINE_PROP_UINT16(_n, _s, _f, _d)                      \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t)
+#define DEFINE_PROP_UINT32(_n, _s, _f, _d)                      \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t)
+#define DEFINE_PROP_HEX32(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
+#define DEFINE_PROP_PTR(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr)
+#define DEFINE_PROP_MAC(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_mac)
+#define DEFINE_PROP_END_OF_LIST()               \
+    {}
+
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
-- 
1.6.2.5