Patchwork [4/9] object: remove object_finalize

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 26, 2012, 3:51 p.m.
Message ID <1345996298-4892-5-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/180104/
State New
Headers show

Comments

Anthony Liguori - Aug. 26, 2012, 3:51 p.m.
Callers should just use object_unref

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c             |    4 ----
 include/qemu/object.h |    9 ---------
 qom/object.c          |    2 +-
 3 files changed, 1 insertions(+), 14 deletions(-)
Andreas Färber - Aug. 27, 2012, 3:01 p.m.
Am 26.08.2012 17:51, schrieb Anthony Liguori:
> Callers should just use object_unref
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c             |    4 ----
>  include/qemu/object.h |    9 ---------
>  qom/object.c          |    2 +-
>  3 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 6b61daa..fdee91f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -678,13 +678,9 @@ static void device_initfn(Object *obj)
>  static void device_finalize(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> -    BusState *bus;
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
>      if (dev->state == DEV_STATE_INITIALIZED) {
> -        while (dev->num_child_bus) {
> -            bus = QLIST_FIRST(&dev->child_bus);
> -        }
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }

This seems to answer my remark on 3/9, should've been squashed into that
one.

> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 487adcd..8bc9935 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -490,15 +490,6 @@ void object_initialize_with_type(void *data, Type type);
>  void object_initialize(void *obj, const char *typename);
>  
>  /**
> - * object_finalize:
> - * @obj: The object to finalize.
> - *
> - * This function destroys and object without freeing the memory associated with
> - * it.
> - */
> -void object_finalize(void *obj);
> -
> -/**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
>   * @typename: The @typename to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 44135c3..1144f79 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -375,7 +375,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
>      }
>  }
>  
> -void object_finalize(void *data)
> +static void object_finalize(void *data)
>  {
>      Object *obj = data;
>      TypeImpl *ti = obj->class->type;

This is what I was referring to with breaking the encapsulation on 3/9:
When we have a PHB with embedded PCIDevice on its PCIBus, as
demonstrated with i440fx and prep_pci, then when doing object_delete()
on the whole thing I expect the main object's finalizer to call
object_finalize() on its embedded childs, forcing their uninit or an
assert if a programming error. Not just an unref that might or might not
finalize it.

If however finalize is called only at refcount 0 then who will unref the
self-created children? Finalize would never be called due to pending
references by its children...

Andreas
Anthony Liguori - Aug. 27, 2012, 3:49 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 26.08.2012 17:51, schrieb Anthony Liguori:
>> Callers should just use object_unref
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/qdev.c             |    4 ----
>>  include/qemu/object.h |    9 ---------
>>  qom/object.c          |    2 +-
>>  3 files changed, 1 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 6b61daa..fdee91f 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -678,13 +678,9 @@ static void device_initfn(Object *obj)
>>  static void device_finalize(Object *obj)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>> -    BusState *bus;
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>  
>>      if (dev->state == DEV_STATE_INITIALIZED) {
>> -        while (dev->num_child_bus) {
>> -            bus = QLIST_FIRST(&dev->child_bus);
>> -        }
>>          if (qdev_get_vmsd(dev)) {
>>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>>          }
>
> This seems to answer my remark on 3/9, should've been squashed into that
> one.
>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index 487adcd..8bc9935 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -490,15 +490,6 @@ void object_initialize_with_type(void *data, Type type);
>>  void object_initialize(void *obj, const char *typename);
>>  
>>  /**
>> - * object_finalize:
>> - * @obj: The object to finalize.
>> - *
>> - * This function destroys and object without freeing the memory associated with
>> - * it.
>> - */
>> -void object_finalize(void *obj);
>> -
>> -/**
>>   * object_dynamic_cast:
>>   * @obj: The object to cast.
>>   * @typename: The @typename to cast to.
>> diff --git a/qom/object.c b/qom/object.c
>> index 44135c3..1144f79 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -375,7 +375,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
>>      }
>>  }
>>  
>> -void object_finalize(void *data)
>> +static void object_finalize(void *data)
>>  {
>>      Object *obj = data;
>>      TypeImpl *ti = obj->class->type;
>
> This is what I was referring to with breaking the encapsulation on 3/9:
> When we have a PHB with embedded PCIDevice on its PCIBus, as
> demonstrated with i440fx and prep_pci, then when doing object_delete()
> on the whole thing I expect the main object's finalizer to call
> object_finalize() on its embedded childs, forcing their uninit or an
> assert if a programming error. Not just an unref that might or might not
> finalize it.
>
> If however finalize is called only at refcount 0 then who will unref the
> self-created children? Finalize would never be called due to pending
> references by its children...

The refs are stored as properties and when properties are removed during
object destruction, those references are dropped.

That is what takes the refcount to 0.  We could add a
g_assert(child->ref == 1) before removing the property to catch
programming errors.

Regards,

Anthony Liguori

>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index 6b61daa..fdee91f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -678,13 +678,9 @@  static void device_initfn(Object *obj)
 static void device_finalize(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
-    BusState *bus;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (dev->state == DEV_STATE_INITIALIZED) {
-        while (dev->num_child_bus) {
-            bus = QLIST_FIRST(&dev->child_bus);
-        }
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
diff --git a/include/qemu/object.h b/include/qemu/object.h
index 487adcd..8bc9935 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -490,15 +490,6 @@  void object_initialize_with_type(void *data, Type type);
 void object_initialize(void *obj, const char *typename);
 
 /**
- * object_finalize:
- * @obj: The object to finalize.
- *
- * This function destroys and object without freeing the memory associated with
- * it.
- */
-void object_finalize(void *obj);
-
-/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
diff --git a/qom/object.c b/qom/object.c
index 44135c3..1144f79 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -375,7 +375,7 @@  static void object_deinit(Object *obj, TypeImpl *type)
     }
 }
 
-void object_finalize(void *data)
+static void object_finalize(void *data)
 {
     Object *obj = data;
     TypeImpl *ti = obj->class->type;