[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 09:36:15AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> 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