On Tue, May 7, 2019 at 9:47 AM Philippe Mathieu-Daudé <phi...@redhat.com> 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 <ehabk...@redhat.com> > Inspired-by: Thomas Huth <th...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> Alistair > --- > 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)) { > -- > 2.20.1 > >