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 <ehabk...@redhat.com> > Inspired-by: Thomas Huth <th...@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> > Signed-off-by: Cédric Le Goater <c...@kaod.org> > Reviewed-by: Joel Stanley <j...@jms.id.au> > --- > v2: > - Described new use of &error_abort (Markus) > - Added Cédric S-o-b (he sent the same 'hw/arm/aspeed_soc.c' patch) > - Added Joel R-b of Cédric patch > --- > hw/arm/aspeed.c | 6 +++--- > hw/arm/aspeed_soc.c | 50 ++++++++++++++++++--------------------------- > 2 files changed, 23 insertions(+), 33 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 1c23ebd9925..f700b7e4fe0 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -159,9 +159,9 @@ static void aspeed_board_init(MachineState *machine, > ram_addr_t max_ram_size; > > bmc = g_new0(AspeedBoardState, 1); > - object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name); > - object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc), > - &error_abort); > + object_initialize_child(OBJECT(machine), "soc", &bmc->soc, > + (sizeof(bmc->soc)), cfg->soc_name, &error_abort, > + NULL); > > sc = ASPEED_SOC_GET_CLASS(&bmc->soc); > > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c > index a27233d4876..faff42b84ad 100644 > --- a/hw/arm/aspeed_soc.c > +++ b/hw/arm/aspeed_soc.c > @@ -106,12 +106,11 @@ static void aspeed_soc_init(Object *obj) > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > int i; > > - object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type); > - object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL); > + object_initialize_child(obj, "cpu", OBJECT(&s->cpu), sizeof(s->cpu), > + sc->info->cpu_type, &error_abort, NULL); > > - object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU); > - object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL); > - qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default()); > + sysbus_init_child_obj(obj, "scu", OBJECT(&s->scu), sizeof(s->scu), > + TYPE_ASPEED_SCU); > qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev", > sc->info->silicon_rev); > object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu), > @@ -121,36 +120,29 @@ static void aspeed_soc_init(Object *obj) > object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu), > "hw-prot-key", &error_abort); > > - object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC); > - object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL); > - qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default()); > + sysbus_init_child_obj(obj, "vic", OBJECT(&s->vic), sizeof(s->vic), > + TYPE_ASPEED_VIC); > > - object_initialize(&s->timerctrl, sizeof(s->timerctrl), > TYPE_ASPEED_TIMER); > - object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL); > + sysbus_init_child_obj(obj, "timerctrl", OBJECT(&s->timerctrl), > + sizeof(s->timerctrl), TYPE_ASPEED_TIMER); > object_property_add_const_link(OBJECT(&s->timerctrl), "scu", > OBJECT(&s->scu), &error_abort); > - qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default()); > > - object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C); > - object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL); > - qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default()); > + sysbus_init_child_obj(obj, "i2c", OBJECT(&s->i2c), sizeof(s->i2c), > + TYPE_ASPEED_I2C); > > - object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename); > - object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL); > - qdev_set_parent_bus(DEVICE(&s->fmc), sysbus_get_default()); > + sysbus_init_child_obj(obj, "fmc", OBJECT(&s->fmc), sizeof(s->fmc), > + sc->info->fmc_typename); > object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs", > &error_abort); > > for (i = 0; i < sc->info->spis_num; i++) { > - object_initialize(&s->spi[i], sizeof(s->spi[i]), > - sc->info->spi_typename[i]); > - object_property_add_child(obj, "spi[*]", OBJECT(&s->spi[i]), NULL); > - qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default()); > + sysbus_init_child_obj(obj, "spi[*]", OBJECT(&s->spi[i]), > + sizeof(s->spi[i]), sc->info->spi_typename[i]); > } > > - object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC); > - object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL); > - qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default()); > + sysbus_init_child_obj(obj, "sdmc", OBJECT(&s->sdmc), sizeof(s->sdmc), > + TYPE_ASPEED_SDMC); > qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev", > sc->info->silicon_rev); > object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc), > @@ -159,16 +151,14 @@ static void aspeed_soc_init(Object *obj) > "max-ram-size", &error_abort); > > for (i = 0; i < sc->info->wdts_num; i++) { > - object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT); > - object_property_add_child(obj, "wdt[*]", OBJECT(&s->wdt[i]), NULL); > - qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default()); > + sysbus_init_child_obj(obj, "wdt[*]", OBJECT(&s->wdt[i]), > + sizeof(s->wdt[i]), TYPE_ASPEED_WDT); > qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev", > sc->info->silicon_rev); > } > > - object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100); > - object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL); > - qdev_set_parent_bus(DEVICE(&s->ftgmac100), sysbus_get_default()); > + sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100), > + sizeof(s->ftgmac100), TYPE_FTGMAC100); > } > > static void aspeed_soc_realize(DeviceState *dev, Error **errp) >
Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>