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

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > 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?
> 
> 
> because api is:
> 
> vstrucut virtio_common *create_virtio_comon(...., size we really want);
> Again, this implements superclass/subclass as well as you can implemnt
> it in C.
> 
> >> 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.
> 
> This is a common patter in qemu.  You can't use qdev/virtio/pci device
> without understanding it.  If your problem is that we need better
> documentation, I agree.  Adding a second idiom for no gain, that is the
> reason that I am agaist.
> 
> >> 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.
> 
> You cant.  Your trick of creating of mallocking in the caller the struct
> don't work for qdev.  qdev is the one that creates it.  Again, any other
> implementation is going to be more complex for no gain.

I can prove that allocation in the caller is better: just looking at
that code made it obvious that no one frees the structure now.  That's
right, the pretty wrappers hide the fact that common_init leaks memory,
my refactoring made this obvious.

> >> 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),
> 
> No, because you don't necesarily know that at that point.
> To add insult to injury, now you can free a device in common code.
> 
> qemu_free() needs to take the beginning of the malloced space.
> 
> In resume:
> 
> type safety:
> - DO_UPCAST() && container_of -> identical (DO_UPCAST uses container_of)
> - wins of using container_of: zero (hypotetical gains when in the future
>   we could use it for .... are that, hypotetical)
> - wins of using DO_UPCAST(): we use the same structure that qdev/pci,
>   not yet a different idiom.

It's best not to do any casts. this is what my patch does:
It removes an upcast during initialization.

> You/me can discuss all day long about point one.  really they have
> exactly the same safety.

The issue is that as long as our design relies on specific
layout in memory, people will abuse this assumption,
with straight casts or punning through void *.
Not relying on struct layout is a simple rule that
makes you think *what kind of object are you passing here*.

> 
> - For you, it is more important to be "future" proof if anybody, anywhere
>   finds a way where it could be used in a different position.
> 
> - For me, it is easier to understand having the same idiom that
>   qdev/pci.  One less idiom in qemu for doing the same -> better.
>   Furthermore, it fits clearly with my model of sub/super class.
> 

As I said, we'll remove the assumption in qdev too, eventually.
And I don't think pci has this problem, except where it uses qdev :)

> Obviously your/my priorities here are different.  None of the solutions
> are better.  Mine is more consistent, that is why I preffer it.  But
> clearly, this discussion is not going anywhere :(
> 
> If you have any other reason that I haven't put here _why_ you want to
> use container_of() instead of DO_UPCAST(), I am open to suggestions.

Ability to put qdev and vdev in the same structure, without
resolving to complex tricks like two structures pointing to each other.

Also look virtio-pci: so VirtIOPCIProxy must
have PCIDevice as first member. Why? Because down there
PCIDevice has qdev inside it, and qdev must be first member.
This is leaking implementation in a very inelegant way.

>  If the argument is to continue to discuss between the two
>  alternatives that I have exposesd, then I think that we have to agree
>  to disagree.
> 
> Later, Juan.

I was thinking about removing the limitation of zero offset
for a while now, and I saw the "no clean way to do this in C"
as a challenge, which made me go ahead and handle this finally:
what I posted, I think, shows a pretty clean way to do this.

I guess before we agree to disagree I'd like to understand the reasons
that you object. Do you generally object to uses of container_of
as opposed to UP_CAST, because you find cast to non-first member
confusing?

-- 
MST