From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MWtBV-0005dc-3q for qemu-devel@nongnu.org; Fri, 31 Jul 2009 10:34:13 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MWtBQ-0005TF-KA for qemu-devel@nongnu.org; Fri, 31 Jul 2009 10:34:12 -0400 Received: from [199.232.76.173] (port=60968 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MWtBQ-0005T1-Ec for qemu-devel@nongnu.org; Fri, 31 Jul 2009 10:34:08 -0400 Received: from mx2.redhat.com ([66.187.237.31]:33790) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MWtBP-0004cS-L2 for qemu-devel@nongnu.org; Fri, 31 Jul 2009 10:34:08 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n6VEXubf002883 for ; Fri, 31 Jul 2009 10:33:56 -0400 Date: Fri, 31 Jul 2009 17:32:51 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [RfC PATCH 2/2] qdev/prop: convert pci.c to helper macros. Message-ID: <20090731143251.GA9233@redhat.com> References: <1249045499-10973-1-git-send-email-kraxel@redhat.com> <1249045499-10973-3-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1249045499-10973-3-git-send-email-kraxel@redhat.com> Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org On Fri, Jul 31, 2009 at 03:04:59PM +0200, Gerd Hoffmann wrote: >=20 > Signed-off-by: Gerd Hoffmann > --- > hw/pci.c | 9 ++------- > hw/pci.h | 2 +- > 2 files changed, 3 insertions(+), 8 deletions(-) >=20 > 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 =3D { > .size =3D sizeof(PCIBus), > .print_dev =3D pcibus_dev_print, > .props =3D (Property[]) { > - { > - .name =3D "addr", > - .info =3D &qdev_prop_pci_devfn, > - .offset =3D offsetof(PCIDevice, devfn), > - .defval =3D (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 =3D &qdev_prop_pci_devfn, - .offset =3D 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 =3D (_prop), \ .offset =3D (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 error= s like: foo.c:26: error: invalid operands to binary - (have =E2=80=98long long in= t *=E2=80=99 and =E2=80=98int *=E2=80=99) This way we need only 1 or 2 macros that everyone can use. > }; > =20 > 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 { > =20 > /* 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]; > =20 > --=20 > 1.6.2.5 >=20 Full self-contained toy example below, in case you want to play with my idea some more: #include #include 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 =3D (_prop), \ .offset =3D (offsetof(_struct, _field) + ( 0 && \ (long)(&(_prop)->type - &((_struct *)0)->_field) \ )) struct prop prop1; struct desc desc1 =3D { QDEV_INFO(&prop1, struct foo, bar2) }; int main() { printf("offset: %d\n", desc1.offset); return 0; }