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

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
>> Michael S. Tsirkin wrote:
>> > That's version 1 of my patch. Version 2 removed even need for macro
>> > completely by moving allocations to the caller.
>> 
>> The downside of moving allocations are: (1) it's one more call in the
>> caller, to allocate the type, (2) it needs a virtual destructor for
>> each type to free the object, which can clutter the code if there is
>> no other reason for virtual destructors.
>> 
>> I don't think those are necessarily bad, but they can remove from the
>> neatness of existing code.  Personally I favour an occasional macro
>> using sizeof/offsetof/container_of if the result is a natural and
>> sensible API to all of its callers.
>> 
>> -- Jamie
>
> We need to free in caller anyway because structure is used
> after cleanup. This can be changed by restructuring code ...
> I don't have a strong preference, anything is better
> than the hack relying on field being at offset 0 in structure.

It is not a hack.

Is the only sane way of sharing virtio common field without having to
export all virtio particular structs :(

I really hate how this is going :(

a- we have code that assumes that virtio is the 1st element of all
   virtio structs.

b- we have a patch that codifies that virtio is used in that way (using
   DO_UPCAST()) (me)

c- we arrived to the point where being it at the beggining of the struct
   is the bigger cast in the universe (I present everybody thinking that
   to look at rest of qemu).  Patch is done that makes it possible to
   alloc memory outside of virtio_common_init().  No code on virtio.c or
   virtio-pci.c is changed to use this new offset.  (michael).

d- Anthony arrives to the discussion stating that it should exist a
   VirtIOPCIBus, that way virtio devices should be hanging of that bus
   (that requires lots of changes).

e- kraxel arrives to the discussion stating that initializing in the
   middle of one struct is the right thing to do, except if you are
   qdev, that then it is not.  I point to his suggestion that it makes
   things still uglier (no answer yet, but it was weekend for him).

f- I show using kraxel example how ugly things go (sizeof (struct A) +
   sizeof (struct B) - sizeof (struct C)).  And here is when things fall
   apart IMHO.  Michael states that it is ok to have to had:
    - an offset field
    - a pointer in one struct to inside the same struct (vdev)
   but assuming and stating and using an offset 0 is ugly.

Have I lost anything?  Is there a realistic way of getting struct
VirtIODevice (and VirtIOBlock, ...) inside VirtIOPCIProxy that don't
imply using an offset + a pointer + exporting the struct definitions
only to calculate its size?

Don't get me wrong.  I am the 1st one hating the relation VirtIO <->
PCI, but if we are going to change it, please do it to something that
its _simpler_ than current approach, not something that is still more
fragile and complex.  I don't doubt that better solutions can exist, but
all that have been proposed are worse/more complex than what we have :(

Later, Juan.