qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Juan Quintela
Subject: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups
Date: Thu, 18 Mar 2010 12:53:06 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux)

"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.

>> 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.

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

- 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.


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.  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.










reply via email to

[Prev in Thread] Current Thread [Next in Thread]