On Mon, Dec 17, 2012 at 06:42:58PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <m...@redhat.com> 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 <m...@redhat.com>
> >> > ---
> >> >
> >> > 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
