* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 11:24 [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting Thomas Huth
@ 2019-02-21 18:08 ` Cédric Le Goater
2019-02-21 18:14 ` Philippe Mathieu-Daudé
2019-02-21 22:29 ` David Gibson
2 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2019-02-21 18:08 UTC (permalink / raw)
To: Thomas Huth, David Gibson, qemu-devel; +Cc: qemu-ppc
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>
Looks fine to me.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> 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),
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 11:24 [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting Thomas Huth
2019-02-21 18:08 ` Cédric Le Goater
@ 2019-02-21 18:14 ` Philippe Mathieu-Daudé
2019-02-21 18:17 ` Cédric Le Goater
2019-02-22 6:20 ` Thomas Huth
2019-02-21 22:29 ` David Gibson
2 siblings, 2 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-21 18:14 UTC (permalink / raw)
To: Thomas Huth, David Gibson, qemu-devel; +Cc: qemu-ppc, Cédric Le Goater
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?
-- >8 --
@@ -244,13 +244,15 @@ static void spapr_xive_instance_init(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);
}
---
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 18:14 ` Philippe Mathieu-Daudé
@ 2019-02-21 18:17 ` Cédric Le Goater
2019-02-21 18:36 ` Philippe Mathieu-Daudé
2019-02-22 6:20 ` Thomas Huth
1 sibling, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2019-02-21 18:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Thomas Huth, David Gibson, qemu-devel
Cc: qemu-ppc
On 2/21/19 7:14 PM, 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?
I have used the same pattern in many files :) In ARM also.
C.
>
> -- >8 --
> @@ -244,13 +244,15 @@ static void spapr_xive_instance_init(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);
> }
> ---
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 18:17 ` Cédric Le Goater
@ 2019-02-21 18:36 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-21 18:36 UTC (permalink / raw)
To: Cédric Le Goater, Thomas Huth, David Gibson, qemu-devel; +Cc: qemu-ppc
On 2/21/19 7:17 PM, Cédric Le Goater wrote:
> On 2/21/19 7:14 PM, 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?
>
> I have used the same pattern in many files :) In ARM also.
OK, tested a cocci script so will send a patch.
Regards,
Phil.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 18:14 ` Philippe Mathieu-Daudé
2019-02-21 18:17 ` Cédric Le Goater
@ 2019-02-22 6:20 ` Thomas Huth
2019-02-24 23:27 ` David Gibson
1 sibling, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2019-02-22 6:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, David Gibson, qemu-devel
Cc: qemu-ppc, Cédric Le Goater
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?
Thomas
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-22 6:20 ` Thomas Huth
@ 2019-02-24 23:27 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2019-02-24 23:27 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-ppc, Cédric Le Goater
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
2019-02-21 11:24 [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting Thomas Huth
2019-02-21 18:08 ` Cédric Le Goater
2019-02-21 18:14 ` Philippe Mathieu-Daudé
@ 2019-02-21 22:29 ` David Gibson
2 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2019-02-21 22:29 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Cédric Le Goater, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 4084 bytes --]
On Thu, Feb 21, 2019 at 12:24:48PM +0100, 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>
Nice cleanup for a common pattern, even if it weren't fixing a bug. Applied.
> ---
> 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),
--
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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread