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

[Juan Quintela]

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

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

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

>> 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 :)

>>  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.  The same for deallocation.  Your suggestion
makes that each device has to handle lifecycle, what is 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.

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