[Qemu-devel] [RfC PATCH 0/2] qdev/prop: macros for creating typechecked properties.

[Qemu-devel] [RfC PATCH 0/2] qdev/prop: macros for creating typechecked properties.

[Gerd Hoffmann]

  Hi folks,

Continuing the property macro debate with some code.  Two patches, one
adding macros, one converting pci.c as an example.

I volunteer to send patches switching all properties over to macros.
But given the discussions we had on the topic so far I'd like to have
a agreement on the route to take before, so my work wouldn't end up in
the trash.

cheers,
  Gerd

[Qemu-devel] [RfC PATCH 1/2] qdev/prop: macros for creating typechecked properties.

[Gerd Hoffmann]

There are DEFINE_PROP_$TYPE("name", struct, field, default) macros for
each property type.  These macros link the qdev_prop_$name struct to the
type used by that property.

typeof(struct->field) is verifyed to be the correct one for the given
property.  This uses gcc extentions, but as it is a check only it could
easily be turned off for non-gcc compilers if needed.

The error message given by the compiler isn't that great, I didn't
manage to make the compiler print something more helpful than
"error: initializer element is not constant".  Ideally I'd like to
somehow generate a message containing the variable referenced by the
macro reference in the failure case, so we could use the variable name
to transport the message ...

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev-addr.h |    3 +++
 hw/qdev.h      |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
index f02bd7a..9568c27 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, target_phys_addr_t)
+
 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 b342afb..4fe534e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,46 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_pci_devfn;
 
+extern int qdev_property_type_check_failed; /* undefined */
+#define typeof_field(type, field) typeof(((type *)0)->field)
+#define type_check(t1,t2) __builtin_choose_expr( \
+        __builtin_types_compatible_p(t1,t2),0,qdev_property_type_check_failed)
+
+#define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
+        .name      = (_name),                                    \
+        .info      = &(_prop),                                   \
+        .offset    = offsetof(_state, _field)                    \
+            + type_check(_type,typeof_field(_state, _field)),    \
+        }
+#define DEFINE_PROP_DEFAULT(_name, _state, _field, _defval, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(_type,typeof_field(_state, _field)),           \
+        .defval    = (_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_UINT64(_n, _s, _f, _d)                      \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
+#define DEFINE_PROP_HEX32(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
+#define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
+#define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
+
+#define DEFINE_PROP_PTR(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)
+#define DEFINE_PROP_MACADDR(_n, _s, _f)         \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, uint8_t[6])
+
+#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

[Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |    9 ++-------
 hw/pci.h |    2 +-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 4d0cdc7..27eac04 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -60,13 +60,8 @@ static struct BusInfo pci_bus_info = {
     .size       = sizeof(PCIBus),
     .print_dev  = pcibus_dev_print,
     .props      = (Property[]) {
-        {
-            .name   = "addr",
-            .info   = &qdev_prop_pci_devfn,
-            .offset = offsetof(PCIDevice, devfn),
-            .defval = (uint32_t[]) { -1 },
-        },
-        {/* end of list */}
+        DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
+        DEFINE_PROP_END_OF_LIST()
     }
 };
 
diff --git a/hw/pci.h b/hw/pci.h
index cbfea6a..a2ec16a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -177,7 +177,7 @@ struct PCIDevice {
 
     /* the following fields are read only */
     PCIBus *bus;
-    int devfn;
+    uint32_t devfn;
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
 
-- 
1.6.2.5

Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros.

[Michael S. Tsirkin]

On Fri, Jul 31, 2009 at 03:04:59PM +0200, Gerd Hoffmann wrote:
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci.c |    9 ++-------
>  hw/pci.h |    2 +-
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 4d0cdc7..27eac04 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -60,13 +60,8 @@ static struct BusInfo pci_bus_info = {
>      .size       = sizeof(PCIBus),
>      .print_dev  = pcibus_dev_print,
>      .props      = (Property[]) {
> -        {
> -            .name   = "addr",
> -            .info   = &qdev_prop_pci_devfn,
> -            .offset = offsetof(PCIDevice, devfn),
> -            .defval = (uint32_t[]) { -1 },
> -        },
> -        {/* end of list */}
> +        DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> +        DEFINE_PROP_END_OF_LIST()
>      }

I think the type safety is an important addition.

Unfortunately there's still duplication - in the macro definition:
DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
which leaves room for mistakes. And there will have to be lots of these
macros, for each type.

Here's an idea: if each property exposed a "type" field, documenting
what type it supports, we could use that for a type-safe macro.  Along
the lines of:

-            .info   = &qdev_prop_pci_devfn,
-            .offset = offsetof(PCIDevice, devfn),
+            QDEV_INFO(&qdev_prop_pci_devfn, PCIDevice, devfn),


Where QDEV_INFO would be defined as something like:
#define QDEV_INFO(_prop, _struct, _field) \
        .info = (_prop), \
        .offset = (offsetof(_struct, _field) + ( 0 && \
           (long)(&(_prop)->type - &((_struct *)0)->_field)) \
        )

Defval could be checked in a similar fashion.

This seems to be free of gcc extensions and gives more or less sane errors like:

foo.c:26: error: invalid operands to binary - (have ‘long long int *’ and ‘int *’)

This way we need only 1 or 2 macros that everyone can use.

>  };
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index cbfea6a..a2ec16a 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -177,7 +177,7 @@ struct PCIDevice {
>  
>      /* the following fields are read only */
>      PCIBus *bus;
> -    int devfn;
> +    uint32_t devfn;

Good catch - with int the value could get sign-extended if we try
to decode just the high bits by >>, and shows importance of
type-checking.

>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>  
> -- 
> 1.6.2.5
> 

Full self-contained toy example below, in case you want to play with
my idea some more:


#include <stdio.h>
#include <stddef.h>

struct prop {
	long long type;
};

struct desc {
	struct prop *info;
	int offset;
};

struct foo {
	int bar1;
	long long bar2;
};

#define QDEV_INFO(_prop, _struct, _field) \
        .info = (_prop), \
        .offset = (offsetof(_struct, _field) + ( 0 && \
           (long)(&(_prop)->type - &((_struct *)0)->_field) \
        ))

struct prop prop1;

struct desc desc1 = {
	QDEV_INFO(&prop1, struct foo, bar2)
};



int main()
{
	printf("offset: %d\n", desc1.offset);
	return 0;
}

Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros.

[Gerd Hoffmann]

On 07/31/09 16:32, Michael S. Tsirkin wrote:
> Unfortunately there's still duplication - in the macro definition:
> DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)

It has to be somewhere ...

> Full self-contained toy example below, in case you want to play with
> my idea some more:

Can you create a working patch against qemu next time please?

> #include<stdio.h>
> #include<stddef.h>
>
> struct prop {
> 	long long type;
> };

This is the PropertyInfo aequivalent, right?

Note we are using the very same PropertyInfo struct for all property 
types we have, thus your idea simply doesn't work.  We could use a 
different struct for each type.  That wouldn't solve the fundamental 
issue though, it would just shift it to another place.

cheers,
   Gerd

Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros.

[Gerd Hoffmann]

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

   Hi,

>             (long)(&(_prop)->type -&((_struct *)0)->_field)) \

> This seems to be free of gcc extensions and gives more or less sane errors like:
>
> foo.c:26: error: invalid operands to binary - (have ‘long long int *’ and ‘int *’)

The "better error message" bit works.  Nice idea.  Incremental patch 
attached.

cheers,
   Gerd

[-- Attachment #2: 0001-better-type-check.patch --]
[-- Type: text/plain, Size: 945 bytes --]

From cf4d63c456250c7e97705338c3c052fe10a5a4b6 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 31 Jul 2009 17:36:01 +0200
Subject: [PATCH] better type check

---
 hw/qdev.h |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/qdev.h b/hw/qdev.h
index 4fe534e..e2703f4 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,10 +156,8 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_pci_devfn;
 
-extern int qdev_property_type_check_failed; /* undefined */
 #define typeof_field(type, field) typeof(((type *)0)->field)
-#define type_check(t1,t2) __builtin_choose_expr( \
-        __builtin_types_compatible_p(t1,t2),0,qdev_property_type_check_failed)
+#define type_check(t1,t2) ((t1*)0 - (t2*)0)
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
-- 
1.6.2.5