[Qemu-devel] [PATCH 0/2][RFC] Introduce macros for setting properties

[Qemu-devel] [PATCH 0/2][RFC] Introduce macros for setting properties

[Anthony Liguori]

This series introduces macros for create qdev properties.  Unlike Gerd's series,
this attempts to simplify the common cases by using type inference and choosing
default names.

A consequence of this is that we need to use proper pointer property types
because GCC's builtin_types_compatible does not consider void * compatible with
arbitrary pointer types.

[Qemu-devel] [PATCH 1/2] Introduce CharDriverState qdev property type

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev-properties.c |   23 +++++++++++++++++++++++
 hw/qdev.h            |    4 ++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 06c25af..bfe77d1 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -145,6 +145,24 @@ PropertyInfo qdev_prop_macaddr = {
     .print = print_mac,
 };
 
+/* -- character device --- */
+
+static int print_chrdev(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    void **ptr = qdev_get_prop_ptr(dev, prop);
+    CharDriverState *chr = *ptr;
+
+    return snprintf(dest, len, "chr: %s", chr->label);
+}
+
+PropertyInfo qdev_prop_chrdev = {
+    .name  = "chrdev",
+    .type  = PROP_TYPE_CHRDEV,
+    .size  = sizeof(CharDriverState*),
+    .print = print_chrdev,
+};
+
+
 /* --- public helpers --- */
 
 static Property *qdev_prop_walk(Property *props, const char *name)
@@ -229,6 +247,11 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
     qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
 }
 
+void qdev_prop_set_chrdev(DeviceState *dev, const char *name, CharDriverState *value)
+{
+    qdev_prop_set(dev, name, &value, PROP_TYPE_CHRDEV);
+}
+
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 {
     char *dst;
diff --git a/hw/qdev.h b/hw/qdev.h
index 11744fa..97722b1 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -3,6 +3,7 @@
 
 #include "hw.h"
 #include "sys-queue.h"
+#include "qemu-char.h"
 
 typedef struct Property Property;
 
@@ -61,6 +62,7 @@ enum PropertyType {
     PROP_TYPE_TADDR,
     PROP_TYPE_MACADDR,
     PROP_TYPE_PTR,
+    PROP_TYPE_CHRDEV,
 };
 
 struct PropertyInfo {
@@ -148,6 +150,7 @@ extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
+extern PropertyInfo qdev_prop_chrdev;
 
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
@@ -157,6 +160,7 @@ 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.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
+void qdev_prop_set_chrdev(DeviceState *dev, const char *name, CharDriverState *chr);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 void qdev_prop_register_compat(CompatProperty *props);
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Anthony Liguori]

This patch introduces macros for defining qdev properties.  The default macro
is clever enough to infer the type of the structure field and to automatically
generate a name for the property.  Additional macros are provided that allow
infered values to be overridden along with a set of macros to define properties
with default values.

This patch converts over syborg and a few other devices as a demonstration.
Final patch will convert everything.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/escc.c             |   50 ++++++++++--------------------------------------
 hw/qdev.h             |   49 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/sun4m.c            |    6 +----
 hw/syborg_fb.c        |   11 +--------
 hw/syborg_interrupt.c |    7 +-----
 hw/syborg_keyboard.c  |    7 +-----
 hw/syborg_pointer.c   |   13 +----------
 hw/syborg_serial.c    |    7 +-----
 hw/syborg_timer.c     |    6 +----
 9 files changed, 69 insertions(+), 87 deletions(-)

diff --git a/hw/escc.c b/hw/escc.c
index 9abd092..ff21277 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -737,8 +737,8 @@ int escc_init(target_phys_addr_t base, qemu_irq irqA, qemu_irq irqB,
     qdev_prop_set_uint32(dev, "disabled", 0);
     qdev_prop_set_uint32(dev, "frequency", clock);
     qdev_prop_set_uint32(dev, "it_shift", it_shift);
-    qdev_prop_set_ptr(dev, "chrB", chrB);
-    qdev_prop_set_ptr(dev, "chrA", chrA);
+    qdev_prop_set_chrdev(dev, "chrB", chrB);
+    qdev_prop_set_chrdev(dev, "chrA", chrA);
     qdev_prop_set_uint32(dev, "chnBtype", ser);
     qdev_prop_set_uint32(dev, "chnAtype", ser);
     qdev_init(dev);
@@ -900,8 +900,8 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
     qdev_prop_set_uint32(dev, "disabled", disabled);
     qdev_prop_set_uint32(dev, "frequency", clock);
     qdev_prop_set_uint32(dev, "it_shift", it_shift);
-    qdev_prop_set_ptr(dev, "chrB", NULL);
-    qdev_prop_set_ptr(dev, "chrA", NULL);
+    qdev_prop_set_chrdev(dev, "chrB", NULL);
+    qdev_prop_set_chrdev(dev, "chrA", NULL);
     qdev_prop_set_uint32(dev, "chnBtype", mouse);
     qdev_prop_set_uint32(dev, "chnAtype", kbd);
     qdev_init(dev);
@@ -952,41 +952,13 @@ static SysBusDeviceInfo escc_info = {
     .qdev.name  = "escc",
     .qdev.size  = sizeof(SerialState),
     .qdev.props = (Property[]) {
-        {
-            .name = "frequency",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, frequency),
-        },
-        {
-            .name = "it_shift",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, it_shift),
-        },
-        {
-            .name = "disabled",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, disabled),
-        },
-        {
-            .name = "chrB",
-            .info = &qdev_prop_ptr,
-            .offset = offsetof(SerialState, chn[1].chr),
-        },
-        {
-            .name = "chrA",
-            .info = &qdev_prop_ptr,
-            .offset = offsetof(SerialState, chn[0].chr),
-        },
-        {
-            .name = "chnBtype",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, chn[1].type),
-        },
-        {
-            .name = "chnAtype",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, chn[0].type),
-        },
+        QDEV_PROP(SerialState, frequency),
+        QDEV_PROP(SerialState, it_shift),
+        QDEV_PROP(SerialState, disabled),
+        QDEV_PROP_NAME(SerialState, chn[1].chr, "chrB"),
+        QDEV_PROP_NAME(SerialState, chn[0].chr, "chrA"),
+        QDEV_PROP_NAME(SerialState, chn[1].type, "chrBtype"),
+        QDEV_PROP_NAME(SerialState, chn[0].type, "chrAtype"),
         {/* end of list */}
     }
 };
diff --git a/hw/qdev.h b/hw/qdev.h
index 97722b1..9ed635c 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -145,6 +145,55 @@ void do_info_qtree(Monitor *mon);
 
 /*** qdev-properties.c ***/
 
+#define CHOOSE(a, b, c) __builtin_choose_expr(a, b, c)
+#define TYPES_COMPAT(a, b) __builtin_types_compatible_p(a, b)
+#define TYPEOF_FIELD(type, field) typeof(((type *)0)->field)
+
+#define QDEV_PROP_TYPE_INFER(type, field)                          \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), uint16_t),  \
+        &qdev_prop_uint16,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), int16_t),   \
+        &qdev_prop_uint16,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), uint32_t),  \
+        &qdev_prop_uint32,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), int32_t),   \
+        &qdev_prop_uint32,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), CharDriverState *), \
+        &qdev_prop_chrdev,                                         \
+        /* force a build break when inference fails */             \
+       (double)3.14159265)))))
+
+#define QDEV_PROP_FULL_DEF(type, field, label, kind, def) \
+    {                                                     \
+        .name = label,                                    \
+        .offset = offsetof(type, field),                  \
+        .info = kind,                                     \
+        .defval = (TYPEOF_FIELD(type, field)[]){def},        \
+    }
+
+#define QDEV_PROP_FULL(type, field, label, kind)      \
+    {                                                 \
+        .name = label,                                \
+        .offset = offsetof(type, field),              \
+        .info = kind,                                 \
+        .defval = 0,                                  \
+    }
+
+#define QDEV_PROP_NAME(type, field, name) \
+    QDEV_PROP_FULL(type, field, name, QDEV_PROP_TYPE_INFER(type, field))
+
+#define QDEV_PROP(type, field)                         \
+    QDEV_PROP_FULL(type, field, stringify(field),      \
+                   QDEV_PROP_TYPE_INFER(type, field))
+
+#define QDEV_PROP_DEFVAL(type, field, defval)                     \
+    QDEV_PROP_FULL_DEF(type, field, stringify(field),             \
+                       QDEV_PROP_TYPE_INFER(type, field), defval)
+
+#define QDEV_PROP_NAME_DEFVAL(type, field, name, defval)          \
+    QDEV_PROP_FULL_DEF(type, field, name,                         \
+                       QDEV_PROP_TYPE_INFER(type, field), defval)
+
 extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_hex32;
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 4954ba3..937960a 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -524,11 +524,7 @@ static SysBusDeviceInfo ram_info = {
     .qdev.name  = "memory",
     .qdev.size  = sizeof(RamDevice),
     .qdev.props = (Property[]) {
-        {
-            .name = "size",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(RamDevice, size),
-        },
+        QDEV_PROP(RamDevice, size),
         {/* end of property list */}
     }
 };
diff --git a/hw/syborg_fb.c b/hw/syborg_fb.c
index 2929ffd..2178adc 100644
--- a/hw/syborg_fb.c
+++ b/hw/syborg_fb.c
@@ -535,15 +535,8 @@ static SysBusDeviceInfo syborg_fb_info = {
     .qdev.name  = "syborg,framebuffer",
     .qdev.size  = sizeof(SyborgFBState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "width",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgFBState, cols),
-        },{
-            .name   = "height",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgFBState, rows),
-        },
+        QDEV_PROP_NAME(SyborgFBState, cols, "width"),
+        QDEV_PROP_NAME(SyborgFBState, rows, "height"),
         {/* end of list */}
     }
 };
diff --git a/hw/syborg_interrupt.c b/hw/syborg_interrupt.c
index a372ec1..32a8d03 100644
--- a/hw/syborg_interrupt.c
+++ b/hw/syborg_interrupt.c
@@ -222,12 +222,7 @@ static SysBusDeviceInfo syborg_int_info = {
     .qdev.name  = "syborg,interrupt",
     .qdev.size  = sizeof(SyborgIntState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "num-interrupts",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgIntState, num_irqs),
-            .defval = (uint32_t[]) { 64 },
-        },
+        QDEV_PROP_NAME_DEFVAL(SyborgIntState, num_irqs, "num-interrupts", 64),
         {/* end of list */}
     }
 };
diff --git a/hw/syborg_keyboard.c b/hw/syborg_keyboard.c
index ffc85a5..a12712d 100644
--- a/hw/syborg_keyboard.c
+++ b/hw/syborg_keyboard.c
@@ -229,12 +229,7 @@ static SysBusDeviceInfo syborg_keyboard_info = {
     .qdev.name  = "syborg,keyboard",
     .qdev.size  = sizeof(SyborgKeyboardState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "fifo-size",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgKeyboardState, fifo_size),
-            .defval = (uint32_t[]) { 16 },
-        },
+        QDEV_PROP_NAME_DEFVAL(SyborgKeyboardState, fifo_size, "fifo-size", 16),
         {/* end of list */}
     }
 };
diff --git a/hw/syborg_pointer.c b/hw/syborg_pointer.c
index edd1f22..e69e579 100644
--- a/hw/syborg_pointer.c
+++ b/hw/syborg_pointer.c
@@ -227,17 +227,8 @@ static SysBusDeviceInfo syborg_pointer_info = {
     .qdev.name  = "syborg,pointer",
     .qdev.size  = sizeof(SyborgPointerState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "fifo-size",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgPointerState, fifo_size),
-            .defval = (uint32_t[]) { 16 },
-        },{
-            .name   = "absolute",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgPointerState, absolute),
-            .defval = (uint32_t[]) { 1 },
-        },
+        QDEV_PROP_NAME_DEFVAL(SyborgPointerState, fifo_size, "fifo-size", 16),
+        QDEV_PROP_DEFVAL(SyborgPointerState, absolute, 1),
         {/* end of list */}
     }
 };
diff --git a/hw/syborg_serial.c b/hw/syborg_serial.c
index f693421..1492a9e 100644
--- a/hw/syborg_serial.c
+++ b/hw/syborg_serial.c
@@ -344,12 +344,7 @@ static SysBusDeviceInfo syborg_serial_info = {
     .qdev.name  = "syborg,serial",
     .qdev.size  = sizeof(SyborgSerialState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "fifo-size",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgSerialState, fifo_size),
-            .defval = (uint32_t[]) { 16 },
-        },
+        QDEV_PROP_NAME_DEFVAL(SyborgSerialState, fifo_size, "fifo-size", 16),
         {/* end of list */}
     }
 };
diff --git a/hw/syborg_timer.c b/hw/syborg_timer.c
index cf96c5f..422037b 100644
--- a/hw/syborg_timer.c
+++ b/hw/syborg_timer.c
@@ -230,11 +230,7 @@ static SysBusDeviceInfo syborg_timer_info = {
     .qdev.name  = "syborg,timer",
     .qdev.size  = sizeof(SyborgTimerState),
     .qdev.props = (Property[]) {
-        {
-            .name   = "frequency",
-            .info   = &qdev_prop_uint32,
-            .offset = offsetof(SyborgTimerState, freq),
-        },
+        QDEV_PROP_NAME(SyborgTimerState, freq, "frequency"),
         {/* end of list */}
     }
 };
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Blue Swirl]

On Fri, Jul 17, 2009 at 5:41 PM, Anthony Liguori<aliguori@us.ibm.com> wrote:
> This patch introduces macros for defining qdev properties.  The default macro
> is clever enough to infer the type of the structure field and to automatically
> generate a name for the property.  Additional macros are provided that allow
> infered values to be overridden along with a set of macros to define properties
> with default values.

Maybe for sake of non-GCC compatibility we should use less clever but
compatible macros, like
QDEV_PROP_NAME_DEFVAL_I32
QDEV_PROP_NAME_DEFVAL_I64
QDEV_PROP_NAME_DEFVAL_PTR
QDEV_PROP_NAME_DEFVAL_CHRDEV
etc?

For example, Sparse does not know about __builtin_choose_expr() and
will probably complain.

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Anthony Liguori]

Blue Swirl wrote:
> On Fri, Jul 17, 2009 at 5:41 PM, Anthony Liguori<aliguori@us.ibm.com> wrote:
>   
>> This patch introduces macros for defining qdev properties.  The default macro
>> is clever enough to infer the type of the structure field and to automatically
>> generate a name for the property.  Additional macros are provided that allow
>> infered values to be overridden along with a set of macros to define properties
>> with default values.
>>     
>
> Maybe for sake of non-GCC compatibility we should use less clever but
> compatible macros, like
>   

If we attempt non-GCC compatibility, we should take a completely 
different approach.

> QDEV_PROP_NAME_DEFVAL_I32
> QDEV_PROP_NAME_DEFVAL_I64
> QDEV_PROP_NAME_DEFVAL_PTR
> QDEV_PROP_NAME_DEFVAL_CHRDEV
> etc?
>
> For example, Sparse does not know about __builtin_choose_expr() and
> will probably complain.
>   
Should be possible to teach sparse about it.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Paul Brook]

On Friday 17 July 2009, Blue Swirl wrote:
> On Fri, Jul 17, 2009 at 5:41 PM, Anthony Liguori<aliguori@us.ibm.com> wrote:
> > This patch introduces macros for defining qdev properties.  The default
> > macro is clever enough to infer the type of the structure field and to
> > automatically generate a name for the property.  Additional macros are
> > provided that allow infered values to be overridden along with a set of
> > macros to define properties with default values.
>
> Maybe for sake of non-GCC compatibility we should use less clever but
> compatible macros, like
> QDEV_PROP_NAME_DEFVAL_I32
> QDEV_PROP_NAME_DEFVAL_I64
> QDEV_PROP_NAME_DEFVAL_PTR
> QDEV_PROP_NAME_DEFVAL_CHRDEV
> etc?

I think I agree. Using GCC extensions for error checking (e.g. DO_UPCAST) or 
performance (__builtin_clz) is fine, but I'm a reluctant to rely on it for 
correct operation.

Paul

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Anthony Liguori]

Paul Brook wrote:
> I think I agree. Using GCC extensions for error checking (e.g. DO_UPCAST) or 
> performance (__builtin_clz) is fine, but I'm a reluctant to rely on it for 
> correct operation.
>   
Practically speaking, we've never supported anything but GCC and I doubt 
we ever will.  In this case, it's an important part of something I'm 
trying to fix about the current property system.

It seems very brittle to me.  You have to specify a type in both the 
state structure and in the property definition.  Those two things are 
very, very far apart in the code.  Right now, the rules about type 
compatible are ill defined which makes it more likely to break beyond 
simple mistakes.  For instance, uint32 is used for uint32_t, int32_t, 
and int.  That seems odd.

I also don't like the fact that we mix field type information with 
display information.  I haven't thought about the best solution to this 
but I think it's either introducing new struct types or adding an 
optional decorator parameter.

The system I'm aiming for looks like this:

typedef struct {
   SysBusDevice parent;

   /* public */
   uint32_t queue_depth;
   uint32_t tx_mitigation_delay;
   CharDriverState *chr;

   /* private */
   ...
} MyDeviceState;

static Property my_device_properties[] = {
  QDEV_PROP(MyDeviceState, queue_depth),
  QDEV_PROP(MyDeviceState, tx_mitigation_delay),
  QDEV_PROP(MyDeviceState, chr),
  {}
};

Where there's a connection between properties and device state fields 
and there's no duplicate type information.  That means that for the most 
part, the rules of type compatible can be ignored by most users.

I'd like to see most uses of QDEV_PROP_NAME eliminated by renaming 
variables and accepting '-' in place of '_'.  We'll always need a way to 
accept default values.

I'm not sure how to do this without GCC extensions.  We could 
potentially add macro decorators and use a sparse-like tool to extract 
property lists automatically from device state.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Blue Swirl]

On Fri, Jul 17, 2009 at 9:11 PM, Anthony Liguori<aliguori@us.ibm.com> wrote:
> Paul Brook wrote:
>>
>> I think I agree. Using GCC extensions for error checking (e.g. DO_UPCAST)
>> or performance (__builtin_clz) is fine, but I'm a reluctant to rely on it
>> for correct operation.
>>
>
> Practically speaking, we've never supported anything but GCC and I doubt we
> ever will.  In this case, it's an important part of something I'm trying to
> fix about the current property system.
>
> It seems very brittle to me.  You have to specify a type in both the state
> structure and in the property definition.  Those two things are very, very
> far apart in the code.  Right now, the rules about type compatible are ill
> defined which makes it more likely to break beyond simple mistakes.  For
> instance, uint32 is used for uint32_t, int32_t, and int.  That seems odd.
>
> I also don't like the fact that we mix field type information with display
> information.  I haven't thought about the best solution to this but I think
> it's either introducing new struct types or adding an optional decorator
> parameter.
>
> The system I'm aiming for looks like this:
>
> typedef struct {
>  SysBusDevice parent;
>
>  /* public */
>  uint32_t queue_depth;
>  uint32_t tx_mitigation_delay;
>  CharDriverState *chr;
>
>  /* private */
>  ...
> } MyDeviceState;
>
> static Property my_device_properties[] = {
>  QDEV_PROP(MyDeviceState, queue_depth),
>  QDEV_PROP(MyDeviceState, tx_mitigation_delay),
>  QDEV_PROP(MyDeviceState, chr),
>  {}
> };
>
> Where there's a connection between properties and device state fields and
> there's no duplicate type information.  That means that for the most part,
> the rules of type compatible can be ignored by most users.
>
> I'd like to see most uses of QDEV_PROP_NAME eliminated by renaming variables
> and accepting '-' in place of '_'.  We'll always need a way to accept
> default values.
>
> I'm not sure how to do this without GCC extensions.  We could potentially
> add macro decorators and use a sparse-like tool to extract property lists
> automatically from device state.

Then there is the template way:

typedef struct MyDeviceState {
#define PROP(type, name) type name;
#define C_TYPE
#include "mydevice_props.hx"
} MyDeviceState;
#undef PROP
#undef C_TYPE

static Property my_device_properties[] = {
#define PROP(type, name) glue(QDEV_PROP_, type)(MyDeviceState, name),
#include "mydevice_props.hx"
 {}
};

Where mydevice_props.hx contains:
#ifdef C_TYPE
#define I32 uint32_t
#define I64 uint64_t
#define CHRDEV CharDriverState *
#endif

PROP(I32, queue_depth)
PROP(I32, tx_mitigation_delay)
PROP(CHRDEV, chr)

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Anthony Liguori]

Blue Swirl wrote:
>> I'm not sure how to do this without GCC extensions.  We could potentially
>> add macro decorators and use a sparse-like tool to extract property lists
>> automatically from device state.
>>     
>
> Then there is the template way:
>   

Yes, I also considered that.

Another option would be comment decorators along with a post-processor.

typedef struct MyDeviceStruct
{
    SysBusDevice parent;

    /* public */
   QDEV_PROP(uint32_t, queue_depth);
   QDEV_PROP(uint32_t, tx_mitigation_delay);
   QDEV_PROP(CharDriverState *, chr);
} MyDeviceStruct;

Normally, we:

#define QDEV_PROP(a, b) a b

But then we could also do something like:

#define QDEV_PROP(a, b) QPROP_CANARY stringify(a) stringify(b)

Then run through CPP and grep 'CANARY | typedef struct' and then parse 
the output to build the table at build time.

Of course, we could just do QDEV_PROP like I originally proposed and 
then if we ever support something other than GCC, we can introduce a CPP 
post-processor to build the tables at compile time.

This is the approach we're taking for constructors after all.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Paul Brook]

On Friday 17 July 2009, Anthony Liguori wrote:
> Blue Swirl wrote:
> >> I'm not sure how to do this without GCC extensions.  We could
> >> potentially add macro decorators and use a sparse-like tool to extract
> >> property lists automatically from device state.
> >
> > Then there is the template way:
>
> Yes, I also considered that.
>
> Another option would be comment decorators along with a post-processor.

QDEV_PROP(uint32, field) and QDEV_PROP_UINT32(field) are pretty much 
isomorphic. Both cases it can be implemented with standard C preprocessor 
functionality, and maybe GCC extensions for compile time checking. I don't 
mind requiring gcc for debug builds and consistency checking.

There is some duplication, with the type specified in both the property macro 
and the structure member definition. As long as we have compile time checking 
I'm willing to accept that.


However I don't think it is possible to implement QDEV_PROP(field) without 
fancy GCC extensions or fairly invasive preprocessing. It feels a little too 
clever for comfort, verging on a custom device description langage.

Paul

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Jamie Lokier]

Paul Brook wrote:
> On Friday 17 July 2009, Anthony Liguori wrote:
> > Blue Swirl wrote:
> > >> I'm not sure how to do this without GCC extensions.  We could
> > >> potentially add macro decorators and use a sparse-like tool to extract
> > >> property lists automatically from device state.
> > >
> > > Then there is the template way:
> >
> > Yes, I also considered that.
> >
> > Another option would be comment decorators along with a post-processor.
> 
> QDEV_PROP(uint32, field) and QDEV_PROP_UINT32(field) are pretty much 
> isomorphic. Both cases it can be implemented with standard C preprocessor 
> functionality, and maybe GCC extensions for compile time checking. I don't 
> mind requiring gcc for debug builds and consistency checking.
> 
> There is some duplication, with the type specified in both the property macro 
> and the structure member definition. As long as we have compile time checking 
> I'm willing to accept that.
> 
> 
> However I don't think it is possible to implement QDEV_PROP(field) without 
> fancy GCC extensions or fairly invasive preprocessing. It feels a little too 
> clever for comfort, verging on a custom device description langage.

In (hopefully) ANSI-portable C code which performs a very similar
function, I got it down to OPTION_SIGNED("name", var),
OPTION_UNSIGNED("name", var), OPTION_BOOL("name", var),
OPTION_STRING("name", var), for the major non-compound types.

For enums, OPTION_ENUM("name", var, ), ENUM("this", THIS),
ENUM("that", THAT), for good value checking, or OPTION_ENUM("name",
var, "this|that|other") if you're confident that the numeric
values are sequential.

Lists, vectors, bounds checks and predicates are easily supplied.

I suspect with C99 varargs macros and compound initialisers (both
supported by GCC) it can be rather tidier, but I didn't explore that.

Then OPTION_SIGNED_RANGE("name", var, 0, 99) and OPTION_UNSIGNED_RANGE
provide nice bounds checks in many cases.

I never found a way to eliminate the SIGNED/UNSIGNED/FLOAT/BOOL
distinction portably, due to the constant-expression rules for global
initialisers.

-- Jamie

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Anthony Liguori]

Jamie Lokier wrote:
> In (hopefully) ANSI-portable C code which performs a very similar
> function, I got it down to OPTION_SIGNED("name", var),
> OPTION_UNSIGNED("name", var), OPTION_BOOL("name", var),
> OPTION_STRING("name", var), for the major non-compound types.
>   
How do you detect the size of the integer?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Introduce macro for defining qdev properties

[Jamie Lokier]

Anthony Liguori wrote:
> Jamie Lokier wrote:
> >In (hopefully) ANSI-portable C code which performs a very similar
> >function, I got it down to OPTION_SIGNED("name", var),
> >OPTION_UNSIGNED("name", var), OPTION_BOOL("name", var),
> >OPTION_STRING("name", var), for the major non-compound types.
> >  
> How do you detect the size of the integer?

Using sizeof(var) :-)

The macros expand to something like

    { name, (void *)&var, sizeof(var),
      SIGNED_MIN_FROM_SIZE(sizeof(var)),
      SIGNED_MAX_FROM_SIZE(sizeof(var)),      
      option_parse_signed },

option_parse_signed is a generic integer parsing function, parsing any
size integer up to intmax_t.  The result is checked against the min
and max fields, which are calculated at compile time.  The _RANGE
version of the macros lets you set them explicitly instead.

Then all parsing functions use the code attached below to store the
value in the correct variable size.

In my app I also include OPTION_SET_{FALSE,TRUE,CONST}, which are good
for options that don't parse an argument; their presence is enough.  I
don't know if that applies to qdev.  I also include help text in each
macro, which proves to be quite nice in several contexts including --help:

    OPTION_UNSIGNED_RANGE("port", opt_port, 0, 65535,
            "the port number to listen on, from 0 to 65535")

Unfortunately although I did find expressions which say if something
is signed or a floating-point value, and evaluate to a constant at
compile time, they aren't "compile-time constant expressions" and so
cannot be used in global initialisers in standard C.  They look like:

#define is_signed(var) ((var) * 0 - 1 < 0)

-- Jamie

ps. Code to store parsed value in arbitrarily sized integer variables:

/* Store an option's value in the option's destination variable, for any
   standard integer or data pointer variable.  The value is expected to
   be appropriate and within range for the destination variable's type. */

static void
option_store_value (const struct option * option, uintmax_t value)
{
  if (option->option_var_ptr != 0)
    {
      char * var_ptr = (char *) option->option_var_ptr;
      const char * value_ptr = (const char *) &value;
      size_t p, size = option->option_var_size;

#ifdef LIBJL_LITTLE_ENDIAN
      /* Nothing to do. */
#else
# ifdef LIBJL_BIG_ENDIAN
      value_ptr += sizeof (value) - size;
# else
#  ifdef LIBJL_PDP_ENDIAN
      value_ptr += (sizeof (value) - size) & ~(size_t)1;
#  else
      /* Should work with any reasonable byte order, even complicated
         ones like 3412 (PDP, VAX) and 43218765 (ARM GCC before 2.8),
         and even when they aren't 8-bit bytes. */
      if (size < sizeof (uintmax_t))
        {
          uintmax_t order = ((uintmax_t) 1 << (size * BITS_PER_BYTE)) - 1;
          const char * order_ptr = (const char *) &order;
          for (p = 0; p < size; p++)
            if (order_ptr [p] != 0)
              break;
          value_ptr += p;
        }
#  endif
# endif
#endif
      for (p = 0; p < size; p++)
        var_ptr [p] = value_ptr [p];
    }
}

[Qemu-devel] Re: [PATCH 2/2] Introduce macro for defining qdev properties

[Gerd Hoffmann]

On 07/17/09 16:41, Anthony Liguori wrote:
> This patch introduces macros for defining qdev properties.  The default macro
> is clever enough to infer the type of the structure field and to automatically
> generate a name for the property.  Additional macros are provided that allow
> infered values to be overridden along with a set of macros to define properties
> with default values.

> +#define TYPEOF_FIELD(type, field) typeof(((type *)0)->field)

Hmm, tried to create something simliar.  Didn't work, looks like I did 
something wrong :-(

> +#define QDEV_PROP_TYPE_INFER(type, field)                          \
> +        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), uint16_t),  \
> +&qdev_prop_uint16,                                         \
> +        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), int16_t),   \
> +&qdev_prop_uint16,                                         \
> +        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), uint32_t),  \
> +&qdev_prop_uint32,                                         \
> +        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), int32_t),   \
> +&qdev_prop_uint32,                                         \
> +        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), CharDriverState *), \
> +&qdev_prop_chrdev,                                         \
> +        /* force a build break when inference fails */             \
> +       (double)3.14159265)))))

You probably want to use something like 
&qdev_prop_not_found_for_this_type instead of pi to get a somewhat 
better error message.

Overall this looks pretty good to me.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Gerd Hoffmann]

On 07/17/09 16:41, Anthony Liguori wrote:

> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>

Hmm, I think that goes into the wrong direction.  Properties should not 
be pointers to internal qemu state.  Yes, there is qemu_prop_ptr, but I 
consider that a temporary stopgap until we can do better.

Instead CharDriverState's should get a name and we need a way to lookup 
them by name.  Then we can store the name instead of a pointer into the 
property.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 07/17/09 16:41, Anthony Liguori wrote:
>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> Hmm, I think that goes into the wrong direction.  Properties should 
> not be pointers to internal qemu state.  Yes, there is qemu_prop_ptr, 
> but I consider that a temporary stopgap until we can do better.
> Instead CharDriverState's should get a name and we need a way to 
> lookup them by name.  Then we can store the name instead of a pointer 
> into the property.

Would you have a property type that was basically a char driver state name?

> cheers,
>   Gerd
>


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Gerd Hoffmann]

On 07/21/09 16:53, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> On 07/17/09 16:41, Anthony Liguori wrote:
>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> Hmm, I think that goes into the wrong direction. Properties should not
>> be pointers to internal qemu state. Yes, there is qemu_prop_ptr, but I
>> consider that a temporary stopgap until we can do better.
>> Instead CharDriverState's should get a name and we need a way to
>> lookup them by name. Then we can store the name instead of a pointer
>> into the property.
>
> Would you have a property type that was basically a char driver state name?

Yes.  Same for drives and others.  A generic string property should do I 
think.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Gerd Hoffmann]

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

On 07/21/09 17:03, Gerd Hoffmann wrote:
> Yes. Same for drives and others. A generic string property should do I
> think.

Something like the attached patch.  Compiles, but not tested yet.

I'm not fully sure the dynamic allocation is actually a good idea though 
as it makes the string property a special case.

Commments?

cheers,
   Gerd

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 2384 bytes --]

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 76699b0..51cb8b3 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -145,6 +145,31 @@ PropertyInfo qdev_prop_macaddr = {
     .print = print_mac,
 };
 
+/* --- string --- */
+
+static int parse_str(DeviceState *dev, Property *prop, const char *str)
+{
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+
+    qemu_free(*ptr);
+    *ptr = qemu_strdup(str);
+    return 0;
+}
+
+static int print_str(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+    return snprintf(dest, len, "%s", *ptr);
+}
+
+PropertyInfo qdev_prop_str = {
+    .name  = "string",
+    .type  = PROP_TYPE_STRING,
+    .size  = sizeof(char*),
+    .parse = parse_str,
+    .print = print_str,
+};
+
 /* --- public helpers --- */
 
 static Property *qdev_prop_walk(Property *props, const char *name)
@@ -229,6 +254,12 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
     qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
 }
 
+void qdev_prop_set_str(DeviceState *dev, const char *name, char *str)
+{
+    char *value = qemu_strdup(str);
+    qdev_prop_set(dev, name, &value, PROP_TYPE_STRING);
+}
+
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 {
     char *dst;
diff --git a/hw/qdev.h b/hw/qdev.h
index 59ac8dc..ec38a37 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -61,6 +61,7 @@ enum PropertyType {
     PROP_TYPE_TADDR,
     PROP_TYPE_MACADDR,
     PROP_TYPE_PTR,
+    PROP_TYPE_STRING,
 };
 
 struct PropertyInfo {
@@ -148,6 +149,7 @@ extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_hex32;
 extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
+extern PropertyInfo qdev_prop_str;
 
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
@@ -157,6 +159,7 @@ 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.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
+void qdev_prop_set_str(DeviceState *dev, const char *name, char *value);
 void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 void qdev_prop_register_compat(CompatProperty *props);

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 07/21/09 16:53, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> On 07/17/09 16:41, Anthony Liguori wrote:
>>>
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> Hmm, I think that goes into the wrong direction. Properties should not
>>> be pointers to internal qemu state. Yes, there is qemu_prop_ptr, but I
>>> consider that a temporary stopgap until we can do better.
>>> Instead CharDriverState's should get a name and we need a way to
>>> lookup them by name. Then we can store the name instead of a pointer
>>> into the property.
>>
>> Would you have a property type that was basically a char driver state 
>> name?
>
> Yes.  Same for drives and others.  A generic string property should do 
> I think.

Couldn't a struct pointer be used for that though?  It can still be 
exposed as a string to the user.

> cheers,
>   Gerd
>


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Gerd Hoffmann]

On 07/21/09 17:23, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>>> Would you have a property type that was basically a char driver state
>>> name?
>>
>> Yes. Same for drives and others. A generic string property should do I
>> think.
>
> Couldn't a struct pointer be used for that though? It can still be
> exposed as a string to the user.

Hmm, didn't think about that possibility.  Somehow makes sense as it 
would move the lookup-by-name into generic code (i.e. 
qemu_prop_chardev->parse).

Not sure what qdev_prop_set_chrdev() should accept then.  struct 
pointer?  string?

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 07/21/09 17:23, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>>> Would you have a property type that was basically a char driver state
>>>> name?
>>>
>>> Yes. Same for drives and others. A generic string property should do I
>>> think.
>>
>> Couldn't a struct pointer be used for that though? It can still be
>> exposed as a string to the user.
>
> Hmm, didn't think about that possibility.  Somehow makes sense as it 
> would move the lookup-by-name into generic code (i.e. 
> qemu_prop_chardev->parse).
>
> Not sure what qdev_prop_set_chrdev() should accept then.  struct 
> pointer?  string?

Good question.  I think it depends on how we connect things.  We could 
introduce another structure for front-ends and then have a separate 
connect mechanism.  The association of names would be transparent to 
qdev/devices.

We could also hand the chrdev structure directly to the device but then 
you have to deal with setting/unsetting.  I think the front-end/back-end 
structure split is the most appealing.

> cheers,
>   Gerd
>


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/2] Introduce CharDriverState qdev property type

[Gerd Hoffmann]

On 07/21/09 17:47, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> Not sure what qdev_prop_set_chrdev() should accept then. struct
>> pointer? string?
>
> Good question. I think it depends on how we connect things. We could
> introduce another structure for front-ends and then have a separate
> connect mechanism. The association of names would be transparent to
> qdev/devices.

Hmm.  When going that route it we might not use attributes at all but 
just tie stuff directly into Device*, i.e. DeviceState gets a backend 
link and DeviceInfo gets a connect callback.

> We could also hand the chrdev structure directly to the device but then
> you have to deal with setting/unsetting.

Optional Property->notify() callback should do for devices which 
actually care (cdrom to signal media change to guest maybe).

cheers,
   Gerd