* [Qemu-devel] [PATCH] hw/ppc: Use object_initialize_child for correct reference counting
@ 2019-02-21 11:24 Thomas Huth
2019-02-21 18:08 ` Cédric Le Goater
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Thomas Huth @ 2019-02-21 11:24 UTC (permalink / raw)
To: David Gibson, qemu-devel; +Cc: Cédric Le Goater, qemu-ppc
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),
--
1.8.3.1
^ 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-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 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
* 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
end of thread, other threads:[~2019-02-24 23:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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-21 18:36 ` Philippe Mathieu-Daudé
2019-02-22 6:20 ` Thomas Huth
2019-02-24 23:27 ` David Gibson
2019-02-21 22:29 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.