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 <address@hidden>
> ---
>  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;
}