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

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> 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.

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.