"Michael S. Tsirkin" <m...@redhat.com> wrote:
> 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.
Until now container_of() was good. Now it is also bad. Don't buy.
DO_UPCAST() is as type safe as your alternative. No differences.
>> >> 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.
This is the same.
>> 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 :) ?
Humm, let me try to understand it. Forcing at position 0 is ugly.
Having to put a new field for an offset, and add/remove the field is
nicer? Give me a break :)
>> >> 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?
No offset assumptions. I check they are 0. See DO_UPCAST().
I guarantee that fields are the same, size the same, and offset is
zero. I can't see where it can fail. Notice that where I put "I" here
means Gerd and the others that implemented qdev, I did almost nothing of it.
>> >> 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
No, it has to be done by each driver. It cant' be done now by the
common code.
> - it is clear that memory is zero initialized
It don't make sense that memory is not 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?
Because it has to know all places where it can be ended/deallocated. In
the other way, common code does it.
>> > 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.
It is more complex. You need to add a new offset field to be able to
free it in common code. To add insult to injury, you have to do there a
cast (without containeroff) to be able to free it there. And once that
you add the offset, the number of lines argument is also lost.
>> (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 :)
Look at it this other way, If I assume that I always have pci, how much
I can simplify?
Later, Juan.
