From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46187) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkulC-0008NK-VZ for qemu-devel@nongnu.org; Tue, 18 Dec 2012 05:50:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkulA-0001zz-CC for qemu-devel@nongnu.org; Tue, 18 Dec 2012 05:50:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3607) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tkul9-0001zp-N2 for qemu-devel@nongnu.org; Tue, 18 Dec 2012 05:50:51 -0500 Date: Tue, 18 Dec 2012 12:53:27 +0200 From: "Michael S. Tsirkin" Message-ID: <20121218105327.GB22586@redhat.com> References: <20121217214042.GA11926@redhat.com> <50CFA3C3.4030707@suse.de> <20121217232725.GA12878@redhat.com> <878v8w5gx9.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <878v8w5gx9.fsf@codemonkey.ws> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv2] virtio: make bindings typesafe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Stefan Hajnoczi , Jan Kiszka , Alexander Graf , qemu-devel@nongnu.org, Christian Borntraeger , pbonzini@redhat.com, Andreas =?iso-8859-1?Q?F=E4rber?= On Mon, Dec 17, 2012 at 06:42:58PM -0600, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: >=20 > > On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas F=E4rber wrote: > >> Am 17.12.2012 22:40, schrieb Michael S. Tsirkin: > >> > Move bindings from opaque to DeviceState. > >> > This gives us better type safety with no performance cost. > >> > Add macros to make future QOM work easier, document > >> > which ones are data-path sensitive. > >> >=20 > >> > Signed-off-by: Michael S. Tsirkin > >> > --- > >> >=20 > >> > Changes from v1: > >> > - Address comment by Anreas F=E4rber: wrap container_of > >> > macros to make future QOM work easier > >> > - make a couple of bindings that v1 missed typesafe: > >> > virtio doesn't use any void * now > >> >=20 > >> > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c > >> > index e0ac2d1..8c693b4 100644 > >> > --- a/hw/s390-virtio-bus.c > >> > +++ b/hw/s390-virtio-bus.c > >> > @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390D= evice *dev, VirtIODevice *vdev) > >> > =20 > >> > bus->dev_offs +=3D dev_len; > >> > =20 > >> > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); > >> > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_T= O_QDEV(dev)); > >>=20 > >> DEVICE(dev) exists for exactly that purpose, and device init is > >> certainly no hot path. Please don't reinvent the wheel for virtio. > > > > OK. > > Though my beef with DEVICE is that it ignores the type > > passed in completely. You can give it int * and it will > > happily cast to devicestate. Your only hope is to > > catch the error at runtime. >=20 > That's a feature. DEVICE can do upcasting and downcasting. There's no > way to do compile time checking of upcasting when >=20 > > It would be better if DEVICE got the name of the > > qdev field, then we could check it's actually DeviceState > > before casting. Yes it would mean a bit of churn if you rename the > > field but it's very rare and trivial to change by a regexp. >=20 > No, it would be much, much worse. You shouldn't have to know what the > layout of the structure is to convert between types. Still I'm pointing out the problems, they are real. Illegal code like DEVICE("foobar") compiles fine and it shouldn't. --=20 MST