Am 04.07.2013 06:46, schrieb liu ping fan:
> On Thu, Jul 4, 2013 at 12:36 AM, Andreas Färber <afaer...@suse.de> wrote:
>> Am 03.07.2013 03:23, schrieb liu ping fan:
>>> On Wed, Jul 3, 2013 at 12:36 AM, Anthony Liguori <anth...@codemonkey.ws>
>>> wrote:
>>>> Paolo Bonzini <pbonz...@redhat.com> writes:
>>>>
>>>>> I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it
>>>>> doesn't really wrap enough to be useful), but I wouldn't oppose it if
>>>>> someone else does it.
>>>>
>>>> I had honestly hoped Object was light enough to be used for this
>>>> purpose. What do you think?
>>>>
>>> I think it is a good idea. So we can consider the object_finalize() as
>>> the place to release everything. Take the DeviceState as example, we
>>> will have
>>>
>>> -- >8 --
>>> Subject: [PATCH] qom: delay DeviceState destructor until object finialize
>>>
>>> Until refcnt->0, we know that the DeviceState can be safely dropped,
>>> so put the destructor there.
>>>
>>> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
>>
>> It would be nice to get CC'ed on such proposals. :)
>>
> I will CC you for qom related topic. :) And according to MAINTAINER,
> I had better CCed maintainer of Device Tree.
Thanks. I was asking because I implemented realized and am working
towards adopting it in the tree.
Device Tree is something different (libfdt/dtc). We do not have
dedicated Device (formerly qdev) maintainers, Paolo and me have been
hacking on it as needed.
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 6985ad8..1f4e5d8 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -794,9 +794,7 @@ static void device_unparent(Object *obj)
>>> bus = QLIST_FIRST(&dev->child_bus);
>>> qbus_free(bus);
>>> }
>>> - if (dev->realized) {
>>> - object_property_set_bool(obj, false, "realized", NULL);
>>> - }
>>> +
>>> if (dev->parent_bus) {
>>> bus_remove_child(dev->parent_bus, dev);
>>> object_unref(OBJECT(dev->parent_bus));
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 803b94b..2c945f0 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -393,6 +393,7 @@ static void object_finalize(void *data)
>>> Object *obj = data;
>>> TypeImpl *ti = obj->class->type;
>>>
>>> + object_property_set_bool(obj, false, "realized", NULL);
>>
>> This is incorrect since we specifically only have "realized" for
>> devices, not for all QOM objects.
>>
>> If we want to move it to the finalizer you'll need to use
>> .instance_finalize on the device type in hw/core/qdev.c.
>> However the derived type's finalizer is run before its parent's, which
> Do you mean the sequence in object_deinit()?
Yes.
>> may lead to realized = false accessing freed memory.
> If my understanding as above is correct, we just need to guarantee
> realized=false (e.g. pci_e1000_uninit )for derived type will only
> free the resource at its layer, and not touch its parent's, then it
> can not access freed memory, right?
For .instance_finalize you are right.
For realized, it is up to the derived type to choose when to call the
parent's realized implementation, e.g. a PCI device's unrealize
implementation will need to call PCIDevice's unrealize after its own
cleanups if it needs to access the config space or other resources
allocated/free at PCIDevice layer. I doubt we can make it a rule not to
touch the parent's resources at all.
But at least today, TYPE_OBJECT does not have an instance_finalize
implementation, so moving realized=false to
hw/core/qdev.c:device_finalize() instead may be an option - hoping Paolo
can comment more on device_unparent() vs. device_finalize() usage.
Regards,
Andreas
>>> object_deinit(obj, ti);
>>> object_property_del_all(obj);
>>>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
