Re: [Qemu-devel] [PATCH v2 01/16] hw/ppc/pnv: Use object_initialize_child for correct reference counting

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4748 bytes --]

On Tue, May 07, 2019 at 06:34:01PM +0200, Philippe Mathieu-Daudé wrote:
> As explained in commit aff39be0ed97:
> 
>   Both functions, object_initialize() and object_property_add_child()
>   increase the reference counter of the new object, so one of the
>   references has to be dropped afterwards to get the reference
>   counting right. Otherwise the child object will not be properly
>   cleaned up when the parent gets destroyed.
>   Thus let's use now object_initialize_child() instead to get the
>   reference counting here right.
> 
> This patch was generated using the following Coccinelle script
> (with a bit of manual fix-up for overly long lines):
> 
>  @use_object_initialize_child@
>  expression parent_obj;
>  expression child_ptr;
>  expression child_name;
>  expression child_type;
>  expression child_size;
>  expression errp;
>  @@
>  (
>  -   object_initialize(child_ptr, child_size, child_type);
>  +   object_initialize_child(parent_obj, child_name,  child_ptr, child_size,
>  +                           child_type, &error_abort, NULL);
>      ... when != parent_obj
>  -   object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), NULL);
>      ...
> ?-   object_unref(OBJECT(child_ptr));
>  |
>  -   object_initialize(child_ptr, child_size, child_type);
>  +   object_initialize_child(parent_obj, child_name,  child_ptr, child_size,
>  +                            child_type, errp, NULL);
>      ... when != parent_obj
>  -   object_property_add_child(parent_obj, child_name, OBJECT(child_ptr), errp);
>      ...
> ?-   object_unref(OBJECT(child_ptr));
>  )
> 
> While the object_initialize() function doesn't take an
> 'Error *errp' argument, the object_initialize_child() does.
> Since this code is used when a machine is created (and is not
> yet running), we deliberately choose to use the &error_abort
> argument instead of ignoring errors if an object creation failed.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Inspired-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/pnv.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index dfb4ea5742c..31aa20ee25d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -994,14 +994,12 @@ static void pnv_chip_quad_realize(Pnv9Chip *chip9, Error **errp)
>          PnvCore *pnv_core = PNV_CORE(chip->cores + (i * 4) * typesize);
>          int core_id = CPU_CORE(pnv_core)->core_id;
>  
> -        object_initialize(eq, sizeof(*eq), TYPE_PNV_QUAD);
>          snprintf(eq_name, sizeof(eq_name), "eq[%d]", core_id);
> +        object_initialize_child(OBJECT(chip), eq_name, eq, sizeof(*eq),
> +                                TYPE_PNV_QUAD, &error_fatal, NULL);
>  
> -        object_property_add_child(OBJECT(chip), eq_name, OBJECT(eq),
> -                                  &error_fatal);
>          object_property_set_int(OBJECT(eq), core_id, "id", &error_fatal);
>          object_property_set_bool(OBJECT(eq), true, "realized", &error_fatal);
> -        object_unref(OBJECT(eq));
>  
>          pnv_xscom_add_subregion(chip, PNV9_XSCOM_EQ_BASE(eq->id),
>                                  &eq->xscom_regs);
> @@ -1165,10 +1163,9 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>              continue;
>          }
>  
> -        object_initialize(pnv_core, typesize, typename);
>          snprintf(core_name, sizeof(core_name), "core[%d]", core_hwid);
> -        object_property_add_child(OBJECT(chip), core_name, OBJECT(pnv_core),
> -                                  &error_fatal);
> +        object_initialize_child(OBJECT(chip), core_name, pnv_core, typesize,
> +                                typename, &error_fatal, NULL);
>          object_property_set_int(OBJECT(pnv_core), smp_threads, "nr-threads",
>                                  &error_fatal);
>          object_property_set_int(OBJECT(pnv_core), core_hwid,
> @@ -1180,7 +1177,6 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>                                         OBJECT(chip), &error_fatal);
>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>                                   &error_fatal);
> -        object_unref(OBJECT(pnv_core));
>  
>          /* Each core has an XSCOM MMIO region */
>          if (!pnv_chip_is_power9(chip)) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]