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

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote:
>> Gerd Hoffmann <address@hidden> wrote:

>
> The whole '1st place' hack is really not necessary, if we know the
> offset we can use container_of.

container_of() don't cut mustard.  The whole point is that virtio-pci
code don't care at all about what virtio specific device it has.  You
should have to use an offset/pointer (take your poison).

>
>> > Just changing it for the snake of change isn't a good reason
>> > either. But if it helps cleaning the code we can change it without
>> > running into trouble.  You can't cast VirtIODevice to VirtIOBlock any
>> > more, but you don't really want to anyway for type checking reasons.
>> 
>> As I have show, from the time that virtio-pci only uses the virtio.c
>> functions (and needs VirtIODevice fields), I don't see how to embed it
>> and share the current functions in virtio-pci.c.
>
> We can just keep the pointer from VirtiPCIProxy to virtio device.

Then there is not much reason of using it embeded in the 1st place.
If we are talking about hackish approachs, a pointer to the middle of
the own struct has lots of points of "hackiness"


>> Notice that what we really want is create the devices in 
>> 
>>         .qdev.name = "virtio-balloon-pci",
>>         .qdev.size = sizeof(VirtIOPCIProxy),
>> 
>> as
>> 
>>         .qdev.name = "virtio-balloon-pci",
>>         .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - 
>> sizeof(VirtIODevice),
>> 
>> And having a single definition of:
>> 
>> struct VirtioPCIProxy {
>>        <current virtioPCIProxy fields>
>>        struct VirtIODevice vdev;
>> }
>> 
>> Yes, it is ugly/subtle as hell, but no other simple way.  You allocate
>> the struct on each virtio-*.c file, where you know the size, and no
>> embed.  Or you embed the struct, and replicate lots of code to have it
>> type safe.
>
> We don't need to drop the vdev pointer if it's helpful.
> If we have it around there will be close to no code
> changes.

And no advantage for the change either :(
Reducing one qemu_mallocz() and nothing else.

>> Any of the two options are worse than current way IMHO.  Having to
>> export VirtIOBlock only to know its size looks wrong in addition.
>> 
>> Depending on size of the last element is ... too hackish?
>
> Yes.

Then I think that changing to not be the 1st element brings us
... nothing?  Or worse, we have to export the VirtIO* structs that are
private now.  I think that it is better just my cleanup, the other
change just complicate things.

Later, Juan.