[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macr
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros. |
Date: |
Fri, 31 Jul 2009 17:32:51 +0300 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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;
}