On Thu, Mar 18, 2010 at 02:43:04PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <m...@redhat.com> 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
