Re: [Qemu-devel] [PATCH v2 08/16] hw/arm: Use object_initialize_child for correct reference counting

[Paolo Bonzini]

On 07/05/19 11:34, 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));
>  )
> 
>  @use_sysbus_init_child_obj@
>  expression parent_obj;
>  expression dev;
>  expression child_ptr;
>  expression child_name;
>  expression child_type;
>  expression child_size;
>  expression errp;
>  @@
>  (
>  -   object_initialize_child(parent_obj, child_name, child_ptr, child_size,
>  -                           child_type, errp, NULL);
>  +   sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
>  +                         child_type);
>      ...
>  -   qdev_set_parent_bus(DEVICE(child_ptr), sysbus_get_default());
>  |
>  -   object_initialize_child(parent_obj, child_name, child_ptr, child_size,
>  -                           child_type, errp, NULL);
>  +   sysbus_init_child_obj(parent_obj, child_name, child_ptr, child_size,
>  +                         child_type);
>  -   dev = DEVICE(child_ptr);
>  -   qdev_set_parent_bus(dev, sysbus_get_default());
>  )
> 
> 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.
> This choice also matches when using sysbus_init_child_obj(),
> since its code is:
> 
>   void sysbus_init_child_obj(Object *parent,
>                              const char *childname, void *child,
>                              size_t childsize, const char *childtype)
>   {
>       object_initialize_child(parent, childname, child, childsize,
>                               childtype, &error_abort, NULL);
> 
>       qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
>   }
> 
> Suggested-by: Eduardo Habkost <address@hidden>
> Inspired-by: Thomas Huth <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> v2:
>  - Tweaked cocci to improve digic_init (Thomas)
>  - Described new use of &error_abort (Markus)
> ---
>  hw/arm/digic.c       | 17 ++++++-----------
>  hw/arm/imx25_pdk.c   |  5 ++---
>  hw/arm/kzm.c         |  5 ++---
>  hw/arm/raspi.c       |  7 +++----
>  hw/arm/sabrelite.c   |  5 ++---
>  hw/arm/xlnx-zcu102.c |  5 ++---
>  hw/arm/xlnx-zynqmp.c |  8 ++++----
>  7 files changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/arm/digic.c b/hw/arm/digic.c
> index 726abb9b485..6ef26c6bac3 100644
> --- a/hw/arm/digic.c
> +++ b/hw/arm/digic.c
> @@ -32,27 +32,22 @@
>  static void digic_init(Object *obj)
>  {
>      DigicState *s = DIGIC(obj);
> -    DeviceState *dev;
>      int i;
>  
> -    object_initialize(&s->cpu, sizeof(s->cpu), "arm946-" TYPE_ARM_CPU);
> -    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
> +                            "arm946-" TYPE_ARM_CPU, &error_abort, NULL);
>  
>      for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
>  #define DIGIC_TIMER_NAME_MLEN    11
>          char name[DIGIC_TIMER_NAME_MLEN];
>  
> -        object_initialize(&s->timer[i], sizeof(s->timer[i]), 
> TYPE_DIGIC_TIMER);
> -        dev = DEVICE(&s->timer[i]);
> -        qdev_set_parent_bus(dev, sysbus_get_default());
>          snprintf(name, DIGIC_TIMER_NAME_MLEN, "timer[%d]", i);
> -        object_property_add_child(obj, name, OBJECT(&s->timer[i]), NULL);
> +        sysbus_init_child_obj(obj, name, &s->timer[i], sizeof(s->timer[i]),
> +                              TYPE_DIGIC_TIMER);
>      }
>  
> -    object_initialize(&s->uart, sizeof(s->uart), TYPE_DIGIC_UART);
> -    dev = DEVICE(&s->uart);
> -    qdev_set_parent_bus(dev, sysbus_get_default());
> -    object_property_add_child(obj, "uart", OBJECT(&s->uart), NULL);
> +    sysbus_init_child_obj(obj, "uart", &s->uart, sizeof(s->uart),
> +                          TYPE_DIGIC_UART);
>  }
>  
>  static void digic_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index 9f3ee147390..eef1b184b0d 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -72,9 +72,8 @@ static void imx25_pdk_init(MachineState *machine)
>      unsigned int alias_offset;
>      int i;
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX25);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_FSL_IMX25, &error_abort, NULL);
>  
>      object_property_set_bool(OBJECT(&s->soc), true, "realized", 
> &error_fatal);
>  
> diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
> index 139934c4ecf..44cba8782bf 100644
> --- a/hw/arm/kzm.c
> +++ b/hw/arm/kzm.c
> @@ -71,9 +71,8 @@ static void kzm_init(MachineState *machine)
>      unsigned int alias_offset;
>      unsigned int i;
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX31);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_FSL_IMX31, &error_abort, NULL);
>  
>      object_property_set_bool(OBJECT(&s->soc), true, "realized", 
> &error_fatal);
>  
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 66899c28dc1..0a6244096cc 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -175,10 +175,9 @@ static void raspi_init(MachineState *machine, int 
> version)
>      BusState *bus;
>      DeviceState *carddev;
>  
> -    object_initialize(&s->soc, sizeof(s->soc),
> -                      version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
> +                            &error_abort, NULL);
>  
>      /* Allocate and map RAM */
>      memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index ee140e5d9eb..f1b00de2294 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -55,9 +55,8 @@ static void sabrelite_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX6);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_FSL_IMX6, &error_abort, NULL);
>  
>      object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
>      if (err != NULL) {
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index b6bc6a93b89..c802f26fbdf 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -91,9 +91,8 @@ static void xlnx_zcu102_init(MachineState *machine)
>      memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
>                                           ram_size);
>  
> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> -                              &error_abort);
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            TYPE_XLNX_ZYNQMP, &error_abort, NULL);
>  
>      object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
>                           "ddr-ram", &error_abort);
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 4f8bc41d9d4..6e991903022 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -191,10 +191,10 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, 
> const char *boot_cpu,
>      for (i = 0; i < num_rpus; i++) {
>          char *name;
>  
> -        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
> -                          "cortex-r5f-" TYPE_ARM_CPU);
> -        object_property_add_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
> -                                  OBJECT(&s->rpu_cpu[i]), &error_abort);
> +        object_initialize_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
> +                                &s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
> +                                "cortex-r5f-" TYPE_ARM_CPU, &error_abort,
> +                                NULL);
>  
>          name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
>          if (strcmp(name, boot_cpu)) {
> 

Reviewed-by: Paolo Bonzini <address@hidden>