Re: [Qemu-devel] [PATCHv2] virtio: make bindings typesafe

[Michael S. Tsirkin]

On Mon, Dec 17, 2012 at 06:42:58PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas Färber 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.
> >> > 
> >> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> >> > ---
> >> > 
> >> > Changes from v1:
> >> >     - Address comment by Anreas Färber: 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
> >> > 
> >> > 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(VirtIOS390Device 
> >> > *dev, VirtIODevice *vdev)
> >> >  
> >> >      bus->dev_offs += dev_len;
> >> >  
> >> > -    virtio_bind_device(vdev, &virtio_s390_bindings, dev);
> >> > +    virtio_bind_device(vdev, &virtio_s390_bindings, 
> >> > VIRTIO_S390_TO_QDEV(dev));
> >> 
> >> 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.
> 
> That's a feature.  DEVICE can do upcasting and downcasting.  There's no
> way to do compile time checking of upcasting when
> 
> > 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.
> 
> 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.

-- 
MST