[Qemu-devel] Re: [PATCH 06/41] virtio: Use DO_UPCAST instead of a cast

[Michael S. Tsirkin]

On Thu, Dec 03, 2009 at 12:56:57PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <address@hidden> wrote:
> >> 
> >> > I don't understand.
> >> > container_of is just more generic than DO_UPCAST.
> >> > So why *ever* use DO_UPCAST? Let's get rid of it.
> 
> ....
> 
> >> See how you create a device of size struct_size, but then you access it
> >> with vdev.  If vdev is _not_ the 1st element of the struct, you have got
> >> corruption.
> >
> > A cleaner solution IMO would be to have callers allocate the memory
> > and pass VirtIODevice * to virtio_common_init.
> 
> Been there, asked for that.  Basically qdev + passing initialized memory
> = nono

It does not have to be initialized.

> >>  DO_UPCAST() prevent you for having that error.
> >
> >
> > If we want to assert specific structure layout, this
> > should be a compile-time check. There's no
> > reason to do this check every time at a random place where
> > DO_UPCAST is called.
> 
> See DO_UPCAST() definition :)
> 
> 
> It is a compile time check.  It just do cpp magic to be sure that things
> are right.  DO_UPCAST() == (cast *) with some typechecking.

Yes, but it gives the error on the wrong place. So it is
only good as long as you do not change the code.

In the example you give with virtio_init_common, there's no UPCAST
to document the layout assumption *in the place where assumption is made*.
On the other hand, DO_UPCAST is scattered all over the code
in places which could work fine without any assumptions.

Let's assume you want to change layout. You find all places
with DO_UPCAST, fix them, and it will compile-but not work.
Let's assume you change all code that makes layout assumptions
to not make them: DO_UPCAST will still be hang around in other
code.

> >> container_of() would have leave you go around, and have a memory
> >> corruption not easy to fix.
> >> 
> >> DO_UPCAST() macro was created just to avoid this kind of errors.
> >> 
> >> Later, Juan.