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

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 08:36:26AM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Tue, Mar 16, 2010 at 07:51:16PM +0100, Juan Quintela wrote:
> >> Hi
> >> 
> >> This series introduces several virtio cleanups:
> >> - add comment to pci (mst)
> >> - tell virtio about DO_UPCAST
> >
> > I think we should move away from struct layout assumptions that
> > DO_UPCAST enforces, and to use container_of where possible.
> > I'll post a series shortly that do this for virtio.
> 
> Not in this case.  qdev needs it to be in that order, and that will not
> change without changing everything again.

I don't think qdev cares about how vdev field is within device
structure, it is not virtio specific.

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

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

> The improtant bit is the patch is:
> 
> +    VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
> +                                            sizeof(struct virtio_blk_config),
> +                                            sizeof(VirtIOBlock));
> +    s = DO_UPCAST(VirtIOBlock, vdev, vdev);
> 
> You can't have a virtio_common_init() that initializes the shared bits
> ad init them in the middle of nowhere.  It _needs_ to be at the
> beginning of the shared struct.

I just sent a patch that removes this assumption.

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

> >> - use QLIST instead of one open list
> >> - virtio-pci/msix: remove duplicated test
> >> 
> >> Please review and apply.
> >> 
> >> This is split for a series previously sent.  Will send the vmstate
> >> conversions as a different series on top of this one.
> >> 
> >> Later, Juan.
> >> 
> >> Juan Quintela (8):
> >>   virtio: Teach virtio-balloon about DO_UPCAST
> >>   virtio: Teach virtio-blk about DO_UPCAST
> >>   virtio: Teach virtio-net about DO_UPCAST
> >>   virtio: Use DO_UPCAST instead of a cast
> >>   virtio-pci: Remove duplicate test
> >>   QLIST: Introduce QLIST_COPY_HEAD
> >>   virtio-blk: change rq type to VirtIOBlockReq
> >>   virtio-blk: use QLIST for the list of requests
> >> 
> >> Michael S. Tsirkin (1):
> >>   qemu/pci: document msix_entries_nr field
> >> 
> >>  hw/msix.c           |    8 -------
> >>  hw/pci.h            |    4 ++-
> >>  hw/virtio-balloon.c |   15 ++++---------
> >>  hw/virtio-blk.c     |   54 
> >> ++++++++++++++++++++++++--------------------------
> >>  hw/virtio-net.c     |   29 +++++++++++----------------
> >>  hw/virtio-pci.c     |    7 +++--
> >>  qemu-queue.h        |    4 +++
> >>  7 files changed, 54 insertions(+), 67 deletions(-)
> >> 
> >>