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

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

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

On Fri, Feb 22, 2019 at 07:20:14AM +0100, Thomas Huth wrote:
> On 21/02/2019 19.14, Philippe Mathieu-Daudé wrote:
> > On 2/21/19 12:24 PM, Thomas Huth wrote:
> >> 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.
> >>
> >> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  hw/ppc/pnv.c     | 12 ++++++------
> >>  hw/ppc/pnv_psi.c |  4 ++--
> >>  hw/ppc/spapr.c   |  6 +++---
> >>  3 files changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index da54086..9e03e9c 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -736,18 +736,18 @@ static void pnv_chip_power8_instance_init(Object *obj)
> >>  {
> >>      Pnv8Chip *chip8 = PNV8_CHIP(obj);
> >>  
> >> -    object_initialize(&chip8->psi, sizeof(chip8->psi), TYPE_PNV_PSI);
> >> -    object_property_add_child(obj, "psi", OBJECT(&chip8->psi), NULL);
> >> +    object_initialize_child(obj, "psi",  &chip8->psi, sizeof(chip8->psi),
> >> +                            TYPE_PNV_PSI, &error_abort, NULL);
> >>      object_property_add_const_link(OBJECT(&chip8->psi), "xics",
> >>                                     OBJECT(qdev_get_machine()), &error_abort);
> >>  
> >> -    object_initialize(&chip8->lpc, sizeof(chip8->lpc), TYPE_PNV_LPC);
> >> -    object_property_add_child(obj, "lpc", OBJECT(&chip8->lpc), NULL);
> >> +    object_initialize_child(obj, "lpc",  &chip8->lpc, sizeof(chip8->lpc),
> >> +                            TYPE_PNV_LPC, &error_abort, NULL);
> >>      object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
> >>                                     OBJECT(&chip8->psi), &error_abort);
> >>  
> >> -    object_initialize(&chip8->occ, sizeof(chip8->occ), TYPE_PNV_OCC);
> >> -    object_property_add_child(obj, "occ", OBJECT(&chip8->occ), NULL);
> >> +    object_initialize_child(obj, "occ",  &chip8->occ, sizeof(chip8->occ),
> >> +                            TYPE_PNV_OCC, &error_abort, NULL);
> >>      object_property_add_const_link(OBJECT(&chip8->occ), "psi",
> >>                                     OBJECT(&chip8->psi), &error_abort);
> >>  }
> >> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> >> index 8ced095..44bc0cb 100644
> >> --- a/hw/ppc/pnv_psi.c
> >> +++ b/hw/ppc/pnv_psi.c
> >> @@ -444,8 +444,8 @@ static void pnv_psi_init(Object *obj)
> >>  {
> >>      PnvPsi *psi = PNV_PSI(obj);
> >>  
> >> -    object_initialize(&psi->ics, sizeof(psi->ics), TYPE_ICS_SIMPLE);
> >> -    object_property_add_child(obj, "ics-psi", OBJECT(&psi->ics), NULL);
> >> +    object_initialize_child(obj, "ics-psi",  &psi->ics, sizeof(psi->ics),
> >> +                            TYPE_ICS_SIMPLE, &error_abort, NULL);
> >>  }
> >>  
> >>  static const uint8_t irq_to_xivr[] = {
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index abf9ebc..6c58dca 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1696,9 +1696,9 @@ static void spapr_create_nvram(sPAPRMachineState *spapr)
> >>  
> >>  static void spapr_rtc_create(sPAPRMachineState *spapr)
> >>  {
> >> -    object_initialize(&spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC);
> >> -    object_property_add_child(OBJECT(spapr), "rtc", OBJECT(&spapr->rtc),
> >> -                              &error_fatal);
> >> +    object_initialize_child(OBJECT(spapr), "rtc",
> >> +                            &spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC,
> >> +                            &error_fatal, NULL);
> >>      object_property_set_bool(OBJECT(&spapr->rtc), true, "realized",
> >>                                &error_fatal);
> >>      object_property_add_alias(OBJECT(spapr), "rtc-time", OBJECT(&spapr->rtc),
> >>
> > 
> > What about intc/spapr_xive.c?
> 
> Good hint. I missed that one since it's in hw/intc, not in hw/ppc...
> 
> David, could you please squash this on top:
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 290a290..ac5f5ef 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -244,13 +244,12 @@ static void spapr_xive_instance_init(Object *obj)
>  {
>      sPAPRXive *xive = SPAPR_XIVE(obj);
>  
> -    object_initialize(&xive->source, sizeof(xive->source), TYPE_XIVE_SOURCE);
> -    object_property_add_child(obj, "source", OBJECT(&xive->source), NULL);
> +    object_initialize_child(obj, "source", &xive->source, sizeof(xive->source),
> +                            TYPE_XIVE_SOURCE, &error_abort, NULL);
>  
> -    object_initialize(&xive->end_source, sizeof(xive->end_source),
> -                      TYPE_XIVE_END_SOURCE);
> -    object_property_add_child(obj, "end_source", OBJECT(&xive->end_source),
> -                              NULL);
> +    object_initialize_child(obj, "end_source", &xive->end_source,
> +                            sizeof(xive->end_source), TYPE_XIVE_END_SOURCE,
> +                            &error_abort, NULL);
>  }
>  
>  static void spapr_xive_realize(DeviceState *dev, Error **errp)
> 
> 
> ... or shall I rather send a v2 or a separate patch for this?

Folded as requested, thanks.

> 
>  Thomas
> 

-- 
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 --]