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

[Michael S. Tsirkin]

On Thu, Mar 18, 2010 at 02:43:04PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Thu, Mar 18, 2010 at 12:53:06PM +0100, Juan Quintela wrote:
> >> 
> >> 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.
> 
> Fully disagree.  A bug is a bug independently of how you find it.
> 
> >> >> 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.
> 
> #ifdef __GNUC__
> #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
>     char __attribute__((unused)) offset_must_be_zero[ \
>         -offsetof(type, field)]; \
>     container_of(dev, type, field);}))
> #else
> #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
> #endif
> 
> 
> DO_UPCAST don't do any cast.  As already say, from type safety
> perspective, there are no differences.  DO_UPCAST is _yours_ patch &
> making sure that field is the 1st one.

Yes but my patch does not do any casting at all, not even
container_of. That is better.

> >> 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*.
> 
> void * leaves you abuse whatever you want.

Right, but at least, we can try to be consistent about which type
you cast it to.

> >> 
> >> - 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.
> 
> I doubt it.  It is a "feature" :)
> 
> > And I don't think pci has this problem, except where it uses qdev :)
> 
> /* Unlink device from bus and free the structure.  */
> void qdev_free(DeviceState *dev)
> {
>     BusState *bus;
> 
>     if (dev->state == DEV_STATE_INITIALIZED) {
>         while (dev->num_child_bus) {
>             bus = QLIST_FIRST(&dev->child_bus);
>             qbus_free(bus);
>         }
>         if (dev->info->vmsd)
>             vmstate_unregister(dev->info->vmsd, dev);
>         if (dev->info->exit)
>             dev->info->exit(dev);
>         if (dev->opts)
>             qemu_opts_del(dev->opts);
>     }
>     qemu_unregister_reset(qdev_reset, dev);
>     QLIST_REMOVE(dev, sibling);
>     qemu_free(dev);
> }
> 
> How are you going to make this work with your design?  if your
> suggestion is to use an offset inside qdev, that is the same that
> forcing it in position zero, just made it uglier.

If it's the same, can we just do it and close the issue :) ?


> >> 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.
> See my example of qdev_free().  That is the biggest example when you
> want it to be the 1st elemement.  But there are other with that assumptions.
> 
> > This is leaking implementation in a very inelegant way.
> 
> Your view varies.  For me, it is fixing a problem in a very elegant way :)

So you want to use a structure, instead of just embedding it
you need to carefully look at implementation to figure out
whether there are offset assumptions ... where is the elegance?

> >>  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.
> 
> No.  You have to do the allocation outside for no good reason, it should
> be done in generic code.

Very good reasons:
- lifetime becomes clearer
- it is clear that memory is zero initialized

> The same for deallocation.  Your suggestion
> makes that each device has to handle lifecycle, what is wrong.

It is trivially simple to understand. So why is it wrong?

> > 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?
> 
> No.  I like container_of() when it is used for good reason.  Notice that
> DO_UPCAST() uses container_of + other thigs to be sure that it is the
> 1st member.
> 
> What I object is:
> - I have a problem that is easily implementable with OOP
> - I can do a simple OOP implementation
> 
> I like to stop here.  Your point is:
> - If I made it more complex, I can simulate multiple inheritance, we
>   still don't need it, but "perhaps" we could need it in the future.

My patch removes lines of code. It is actually simpler than
what we had: no casts, no assumptions.

> (Yes, that is paraphrasing yourself in a funny way, pardon for the
> license )
> 
> about vdev & pcidev.  I still think that a lot of things would be easier
> if a vdev device is a pci_device _always_, not just some times.  And
> VirtIOPCIProxy shows it.
> 
> Later, Juan.

Yes. But we don't always have pci. So the world we model does
not match single inheritance: a cow is both a mammal and a quadruped,
even if java programmers prefer it to be a mammal first of all :)

-- 
MST