On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> >> Look at the rest of hw/*.
> >
> > I think you mean that a similar assumption is made by lots of
> > other code. Does not mean it's always a good idea and that
> > we need to force this assumption everywhere.
>
> Principle of Least Surprise. as posted in the other email, removing
> that assumption don't bring us anything and just make code more complex
> (not a huge ammount, but more complex).
>
Well, you can not put both vdev and qdev in the same device
as both want to be the first field. If you try, you get
a big surprise :)
> >> The only "sane" way of doing OOP on C is to use the super-class memmbers
> >> as the
> >> 1st member of the sub-classes. That will not change anytime soon. And
> >> trying to "emulate" multiple inheritance in C is completely "insane".
> >
> > container_of does it and I think it's sane.
> > People like comparing it to inheritance but for me it's just
> > using a field in a structure. multiple fields in a structure are insane?
>
> It is inheritance.
>
> we assume in virtio_common_init() can be called for all virtio devices.
> That means that all virtio devices have "something" in common.
> That is the basic concept of super-class and sub-class. Yes, C sucks
> for describing that kind of relationships, but that is a different matter.
>
> >> This "assumption" is used for all the
> >> code left and right. It is an essentioal part of the qdev design, not
> >> something that can be changed easily.
> >
> > I do not think it is essential at all. We just add an offset.
> > Yes we make such assumptions a lot but it's a bug, not a feature.
>
> Clearly, we don't agree here. For me it is a "feature" that all virtio
> devices are in the way:
>
> struct virtio_common {
> ...
> }
>
> struct virtio_specific {
> struct virtio_common common;
> <specific fields>
> }
>
> And then I can pass virtio_specific pointers to virtio_common routines
> and things just work.
With casts? Why not pass in &specific->common?
> Special importance when we have callbacks, and
> virtio_specific parts are only used in virtio_specific.c file.
You need a cast anyway: container_of or UP_CAST. With layout
assumptions people tend to be lazy and use C casts, and it
kind of works, until it breaks in surprising ways.
> Why do you want to break that assumption that is used (in a good place)
> in a lot of qemu code (qdev "requires" it)?
Not break, lift the assumption.
> For me is a clear case of
> coherence. Virtio* can live with container_of() instead of DO_UPCAST()
> because they are not exposed (directly) through qdev. Then mark it as
> different that everything else, indeed if it don't bring us anything,
> just to confuse new users. This is one of the features that I hate
> more, searching for how to use a qemu api because from only the
> prototypes it is not clear, and just pick an example, and that one uses
> it in a non-conventional way.
>
> Later, Juan.
I think we should just fix qdev. All we need to do is replace
.qdev.size = sizeof(VirtIOPCIProxy),
with
DEFINE_QDEV(VirtIOPCIProxy, qdev),
--
MST
