* [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
@ 2020-02-12 3:36 kuhn.chenqun
2020-02-12 6:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: kuhn.chenqun @ 2020-02-12 3:36 UTC (permalink / raw)
To: qemu-devel, i.mitsyanko, peter.maydell
Cc: qemu-trivial, Chen Qun, zhang.zhanghailiang
From: Chen Qun <kuhn.chenqun@huawei.com>
It's easy to reproduce as follow:
virsh qemu-monitor-command vm1 --pretty '{"execute": "device-list-properties",
"arguments":{"typename":"exynos4210.uart"}}'
ASAN shows memory leak stack:
#1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
#2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
#3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
#4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
#5 0xaaad270beee3 in exynos4210_uart_init /qemu/hw/char/exynos4210_uart.c:677
#6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
#7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
#8 0xaaad2755df2f in qmp_device_list_properties /qemu/qom/qom-qmp-cmds.c:152
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
hw/char/exynos4210_uart.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 25d6588e41..5048db5410 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
SysBusDevice *dev = SYS_BUS_DEVICE(obj);
Exynos4210UartState *s = EXYNOS4210_UART(dev);
- s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
- exynos4210_uart_timeout_int, s);
- s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
-
/* memory mapping */
memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
"exynos4210.uart", EXYNOS4210_UART_REGS_MEM_SIZE);
@@ -691,6 +687,10 @@ static void exynos4210_uart_realize(DeviceState *dev, Error **errp)
{
Exynos4210UartState *s = EXYNOS4210_UART(dev);
+ s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ exynos4210_uart_timeout_int, s);
+ s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
+
qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
exynos4210_uart_receive, exynos4210_uart_event,
NULL, s, NULL, true);
--
2.23.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 3:36 [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init kuhn.chenqun
@ 2020-02-12 6:16 ` Philippe Mathieu-Daudé
2020-02-12 6:44 ` Chenqun (kuhn)
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-12 6:16 UTC (permalink / raw)
To: kuhn.chenqun, qemu-devel, i.mitsyanko, peter.maydell
Cc: qemu-trivial, zhang.zhanghailiang
On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
> From: Chen Qun <kuhn.chenqun@huawei.com>
>
> It's easy to reproduce as follow:
> virsh qemu-monitor-command vm1 --pretty '{"execute": "device-list-properties",
> "arguments":{"typename":"exynos4210.uart"}}'
>
> ASAN shows memory leak stack:
> #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
> #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
> #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
> #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
> #5 0xaaad270beee3 in exynos4210_uart_init /qemu/hw/char/exynos4210_uart.c:677
> #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
> #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
> #8 0xaaad2755df2f in qmp_device_list_properties /qemu/qom/qom-qmp-cmds.c:152
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> hw/char/exynos4210_uart.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index 25d6588e41..5048db5410 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
> SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>
> - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> - exynos4210_uart_timeout_int, s);
> - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
Why are you moving s->wordtime from init() to realize()?
> -
> /* memory mapping */
> memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
> "exynos4210.uart", EXYNOS4210_UART_REGS_MEM_SIZE);
> @@ -691,6 +687,10 @@ static void exynos4210_uart_realize(DeviceState *dev, Error **errp)
> {
> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>
> + s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + exynos4210_uart_timeout_int, s);
> + s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
> +
> qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
> exynos4210_uart_receive, exynos4210_uart_event,
> NULL, s, NULL, true);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 6:16 ` Philippe Mathieu-Daudé
@ 2020-02-12 6:44 ` Chenqun (kuhn)
2020-02-12 7:39 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 9+ messages in thread
From: Chenqun (kuhn) @ 2020-02-12 6:44 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, i.mitsyanko, peter.maydell
Cc: qemu-trivial, Zhanghailiang
>-----Original Message-----
>From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>Sent: Wednesday, February 12, 2020 2:16 PM
>To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
>Cc: qemu-trivial@nongnu.org; Zhanghailiang
><zhang.zhanghailiang@huawei.com>
>Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>exynos4210_uart_init
>
>On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
>> From: Chen Qun <kuhn.chenqun@huawei.com>
>>
>> It's easy to reproduce as follow:
>> virsh qemu-monitor-command vm1 --pretty '{"execute":
>> "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
>>
>> ASAN shows memory leak stack:
>> #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>> #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
>> #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>> #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
>> #5 0xaaad270beee3 in exynos4210_uart_init
>/qemu/hw/char/exynos4210_uart.c:677
>> #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
>> #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
>> #8 0xaaad2755df2f in qmp_device_list_properties
>> /qemu/qom/qom-qmp-cmds.c:152
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> hw/char/exynos4210_uart.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>> index 25d6588e41..5048db5410 100644
>> --- a/hw/char/exynos4210_uart.c
>> +++ b/hw/char/exynos4210_uart.c
>> @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>> SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>
>> - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> - exynos4210_uart_timeout_int, s);
>> - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>
>Why are you moving s->wordtime from init() to realize()?
>
Hi Philippe, thanks for your reply!
Because I found the variable wordtime is usually used with fifo_timeout_timer.
Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
I didn't find anything wrong with wordtime in the realize().
Does it have any other effects?
Thanks.
>> -
>> /* memory mapping */
>> memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
>> "exynos4210.uart",
>> EXYNOS4210_UART_REGS_MEM_SIZE); @@ -691,6 +687,10 @@ static void
>exynos4210_uart_realize(DeviceState *dev, Error **errp)
>> {
>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>
>> + s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> + exynos4210_uart_timeout_int, s);
>> + s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>> +
>> qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
>> exynos4210_uart_receive, exynos4210_uart_event,
>> NULL, s, NULL, true);
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 6:44 ` Chenqun (kuhn)
@ 2020-02-12 7:39 ` Philippe Mathieu-Daudé
2020-02-12 16:19 ` Eduardo Habkost
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-12 7:39 UTC (permalink / raw)
To: Chenqun (kuhn), qemu-devel, i.mitsyanko, peter.maydell
Cc: qemu-trivial, Markus Armbruster, Zhanghailiang, Eduardo Habkost
Cc'ing Eduardo & Markus.
On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> Sent: Wednesday, February 12, 2020 2:16 PM
>> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>> devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
>> Cc: qemu-trivial@nongnu.org; Zhanghailiang
>> <zhang.zhanghailiang@huawei.com>
>> Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>> exynos4210_uart_init
>>
>> On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
>>> From: Chen Qun <kuhn.chenqun@huawei.com>
>>>
>>> It's easy to reproduce as follow:
>>> virsh qemu-monitor-command vm1 --pretty '{"execute":
>>> "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
>>>
>>> ASAN shows memory leak stack:
>>> #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>>> #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
>>> #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>>> #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
>>> #5 0xaaad270beee3 in exynos4210_uart_init
>> /qemu/hw/char/exynos4210_uart.c:677
>>> #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
>>> #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
>>> #8 0xaaad2755df2f in qmp_device_list_properties
>>> /qemu/qom/qom-qmp-cmds.c:152
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>> ---
>>> hw/char/exynos4210_uart.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>>> index 25d6588e41..5048db5410 100644
>>> --- a/hw/char/exynos4210_uart.c
>>> +++ b/hw/char/exynos4210_uart.c
>>> @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>>> SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>>
>>> - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>> - exynos4210_uart_timeout_int, s);
>>> - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>>
>> Why are you moving s->wordtime from init() to realize()?
>>
> Hi Philippe, thanks for your reply!
>
> Because I found the variable wordtime is usually used with fifo_timeout_timer.
> Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
>
> I didn't find anything wrong with wordtime in the realize().
> Does it have any other effects?
IIUC when we use both init() and realize(), realize() should only
contains on code that consumes the object properties... But maybe the
design is not clear. Then why not move all the init() code to realize()?
>>> -
>>> /* memory mapping */
>>> memory_region_init_io(&s->iomem, obj, &exynos4210_uart_ops, s,
>>> "exynos4210.uart",
>>> EXYNOS4210_UART_REGS_MEM_SIZE); @@ -691,6 +687,10 @@ static void
>> exynos4210_uart_realize(DeviceState *dev, Error **errp)
>>> {
>>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>>
>>> + s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>> + exynos4210_uart_timeout_int, s);
>>> + s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>>> +
>>> qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
>>> exynos4210_uart_receive, exynos4210_uart_event,
>>> NULL, s, NULL, true);
>>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 7:39 ` Philippe Mathieu-Daudé
@ 2020-02-12 16:19 ` Eduardo Habkost
2020-02-13 2:10 ` Chenqun (kuhn)
2020-02-13 14:28 ` Markus Armbruster
0 siblings, 2 replies; 9+ messages in thread
From: Eduardo Habkost @ 2020-02-12 16:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: peter.maydell, Zhanghailiang, i.mitsyanko, qemu-devel,
Markus Armbruster, qemu-trivial, Chenqun (kuhn)
On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Eduardo & Markus.
>
> On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
> > > -----Original Message-----
> > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> > > Sent: Wednesday, February 12, 2020 2:16 PM
> > > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
> > > devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
> > > Cc: qemu-trivial@nongnu.org; Zhanghailiang
> > > <zhang.zhanghailiang@huawei.com>
> > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
> > > exynos4210_uart_init
> > >
> > > On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
> > > > From: Chen Qun <kuhn.chenqun@huawei.com>
> > > >
> > > > It's easy to reproduce as follow:
> > > > virsh qemu-monitor-command vm1 --pretty '{"execute":
> > > > "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
> > > >
> > > > ASAN shows memory leak stack:
> > > > #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
> > > > #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
> > > > #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
> > > > #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
> > > > #5 0xaaad270beee3 in exynos4210_uart_init
> > > /qemu/hw/char/exynos4210_uart.c:677
> > > > #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
> > > > #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
> > > > #8 0xaaad2755df2f in qmp_device_list_properties
> > > > /qemu/qom/qom-qmp-cmds.c:152
> > > >
> > > > Reported-by: Euler Robot <euler.robot@huawei.com>
> > > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > > > ---
> > > > hw/char/exynos4210_uart.c | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > > > index 25d6588e41..5048db5410 100644
> > > > --- a/hw/char/exynos4210_uart.c
> > > > +++ b/hw/char/exynos4210_uart.c
> > > > @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
> > > > SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > > > Exynos4210UartState *s = EXYNOS4210_UART(dev);
> > > >
> > > > - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > > > - exynos4210_uart_timeout_int, s);
> > > > - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
> > >
> > > Why are you moving s->wordtime from init() to realize()?
> > >
> > Hi Philippe, thanks for your reply!
> >
> > Because I found the variable wordtime is usually used with fifo_timeout_timer.
> > Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
> >
> > I didn't find anything wrong with wordtime in the realize().
> > Does it have any other effects?
>
> IIUC when we use both init() and realize(), realize() should only contains
> on code that consumes the object properties... But maybe the design is not
> clear. Then why not move all the init() code to realize()?
Normally I would recommend the opposite: delay as much as
possible to realize(), to avoid unwanted side effects when (e.g.)
running qom-list-properties.
But as s->wordtime is a simple struct field (that we could even
decide to expose to the outside as a read-only QOM property), it
doesn't really matter. Personally, I would keep it where it is
just to avoid churn.
--
Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 16:19 ` Eduardo Habkost
@ 2020-02-13 2:10 ` Chenqun (kuhn)
2020-02-13 14:28 ` Markus Armbruster
1 sibling, 0 replies; 9+ messages in thread
From: Chenqun (kuhn) @ 2020-02-13 2:10 UTC (permalink / raw)
To: Eduardo Habkost, Philippe Mathieu-Daudé
Cc: peter.maydell, Zhanghailiang, i.mitsyanko, qemu-devel,
Markus Armbruster, qemu-trivial
>-----Original Message-----
>From: Eduardo Habkost [mailto:ehabkost@redhat.com]
>Sent: Thursday, February 13, 2020 12:20 AM
>To: Philippe Mathieu-Daudé <philmd@redhat.com>
>Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org;
>qemu-trivial@nongnu.org; Zhanghailiang
><zhang.zhanghailiang@huawei.com>; Markus Armbruster
><armbru@redhat.com>
>Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>exynos4210_uart_init
>
>On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
>> Cc'ing Eduardo & Markus.
>>
>> On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
>> > > -----Original Message-----
>> > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> > > Sent: Wednesday, February 12, 2020 2:16 PM
>> > > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>> > > devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
>> > > Cc: qemu-trivial@nongnu.org; Zhanghailiang
>> > > <zhang.zhanghailiang@huawei.com>
>> > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>> > > exynos4210_uart_init
>> > >
>> > > On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
>> > > > From: Chen Qun <kuhn.chenqun@huawei.com>
>> > > >
>> > > > It's easy to reproduce as follow:
>> > > > virsh qemu-monitor-command vm1 --pretty '{"execute":
>> > > > "device-list-properties",
>"arguments":{"typename":"exynos4210.uart"}}'
>> > > >
>> > > > ASAN shows memory leak stack:
>> > > > #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>> > > > #2 0xaaad270beee3 in timer_new_full
>/qemu/include/qemu/timer.h:530
>> > > > #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>> > > > #4 0xaaad270beee3 in timer_new_ns
>/qemu/include/qemu/timer.h:569
>> > > > #5 0xaaad270beee3 in exynos4210_uart_init
>> > > /qemu/hw/char/exynos4210_uart.c:677
>> > > > #6 0xaaad275c8f4f in object_initialize_with_type
>/qemu/qom/object.c:516
>> > > > #7 0xaaad275c91bb in object_new_with_type
>/qemu/qom/object.c:684
>> > > > #8 0xaaad2755df2f in qmp_device_list_properties
>> > > > /qemu/qom/qom-qmp-cmds.c:152
>> > > >
>> > > > Reported-by: Euler Robot <euler.robot@huawei.com>
>> > > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> > > > ---
>> > > > hw/char/exynos4210_uart.c | 8 ++++----
>> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
>> > > >
>> > > > diff --git a/hw/char/exynos4210_uart.c
>> > > > b/hw/char/exynos4210_uart.c index 25d6588e41..5048db5410 100644
>> > > > --- a/hw/char/exynos4210_uart.c
>> > > > +++ b/hw/char/exynos4210_uart.c
>> > > > @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>> > > > SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> > > > Exynos4210UartState *s = EXYNOS4210_UART(dev);
>> > > >
>> > > > - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> > > > - exynos4210_uart_timeout_int, s);
>> > > > - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>> > >
>> > > Why are you moving s->wordtime from init() to realize()?
>> > >
>> > Hi Philippe, thanks for your reply!
>> >
>> > Because I found the variable wordtime is usually used with
>fifo_timeout_timer.
>> > Eg, they are used together in the exynos4210_uart_rx_timeout_set
>function.
>> >
>> > I didn't find anything wrong with wordtime in the realize().
>> > Does it have any other effects?
>>
>> IIUC when we use both init() and realize(), realize() should only
>> contains on code that consumes the object properties... But maybe the
>> design is not clear. Then why not move all the init() code to realize()?
>
>Normally I would recommend the opposite: delay as much as possible to
>realize(), to avoid unwanted side effects when (e.g.) running qom-list-
>properties.
>
>But as s->wordtime is a simple struct field (that we could even decide to
>expose to the outside as a read-only QOM property), it doesn't really matter.
>Personally, I would keep it where it is just to avoid churn.
>
OK, Let's keep s->wordtime in init().
I will change it in next version.
Thanks.
>--
>Eduardo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-12 16:19 ` Eduardo Habkost
2020-02-13 2:10 ` Chenqun (kuhn)
@ 2020-02-13 14:28 ` Markus Armbruster
2020-02-13 16:32 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2020-02-13 14:28 UTC (permalink / raw)
To: Eduardo Habkost
Cc: peter.maydell, Zhanghailiang, i.mitsyanko, qemu-devel,
qemu-trivial, Chenqun (kuhn), Philippe Mathieu-Daudé
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
>> Cc'ing Eduardo & Markus.
>>
>> On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
>> > > -----Original Message-----
>> > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> > > Sent: Wednesday, February 12, 2020 2:16 PM
>> > > To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>> > > devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
>> > > Cc: qemu-trivial@nongnu.org; Zhanghailiang
>> > > <zhang.zhanghailiang@huawei.com>
>> > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>> > > exynos4210_uart_init
>> > >
>> > > On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
>> > > > From: Chen Qun <kuhn.chenqun@huawei.com>
>> > > >
>> > > > It's easy to reproduce as follow:
>> > > > virsh qemu-monitor-command vm1 --pretty '{"execute":
>> > > > "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
>> > > >
>> > > > ASAN shows memory leak stack:
>> > > > #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>> > > > #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
>> > > > #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>> > > > #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
>> > > > #5 0xaaad270beee3 in exynos4210_uart_init
>> > > /qemu/hw/char/exynos4210_uart.c:677
>> > > > #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
>> > > > #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
>> > > > #8 0xaaad2755df2f in qmp_device_list_properties
>> > > > /qemu/qom/qom-qmp-cmds.c:152
>> > > >
>> > > > Reported-by: Euler Robot <euler.robot@huawei.com>
>> > > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> > > > ---
>> > > > hw/char/exynos4210_uart.c | 8 ++++----
>> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
>> > > >
>> > > > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>> > > > index 25d6588e41..5048db5410 100644
>> > > > --- a/hw/char/exynos4210_uart.c
>> > > > +++ b/hw/char/exynos4210_uart.c
>> > > > @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>> > > > SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> > > > Exynos4210UartState *s = EXYNOS4210_UART(dev);
>> > > >
>> > > > - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> > > > - exynos4210_uart_timeout_int, s);
>> > > > - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>> > >
>> > > Why are you moving s->wordtime from init() to realize()?
>> > >
>> > Hi Philippe, thanks for your reply!
>> >
>> > Because I found the variable wordtime is usually used with fifo_timeout_timer.
>> > Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
>> >
>> > I didn't find anything wrong with wordtime in the realize().
>> > Does it have any other effects?
>>
>> IIUC when we use both init() and realize(), realize() should only contains
>> on code that consumes the object properties... But maybe the design is not
>> clear. Then why not move all the init() code to realize()?
>
> Normally I would recommend the opposite: delay as much as
> possible to realize(), to avoid unwanted side effects when (e.g.)
> running qom-list-properties.
Sadly, our documentation on device initialization and realization is
rather sparse, and developers are left guessing. Their guesses are
often based on what existing code does. Some of the existing code even
gets things right.
A few rules from the top of my head:
* Creating and immediately destroying an object must be safe and free of
side effects: initialization may only touch the object itself, and
finalization must clean up everything initialization creates.
* unrealize() must clean up everything realize() creates.
* Since initialization cannot fail, code that needs to fail gracefully
must live in realize().
* Since property values get set between initialization and realization,
code that uses property values must live in realize().
* Dynamic properties have to be created in initialization to be visible
in introspection.
> But as s->wordtime is a simple struct field (that we could even
> decide to expose to the outside as a read-only QOM property), it
> doesn't really matter. Personally, I would keep it where it is
> just to avoid churn.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-13 14:28 ` Markus Armbruster
@ 2020-02-13 16:32 ` Philippe Mathieu-Daudé
2020-02-13 16:37 ` Peter Maydell
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-13 16:32 UTC (permalink / raw)
To: Markus Armbruster, Eduardo Habkost
Cc: peter.maydell, Zhanghailiang, i.mitsyanko, qemu-devel,
qemu-trivial, Chenqun (kuhn)
On 2/13/20 3:28 PM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Eduardo & Markus.
>>>
>>> On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
>>>>> -----Original Message-----
>>>>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>>>>> Sent: Wednesday, February 12, 2020 2:16 PM
>>>>> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-
>>>>> devel@nongnu.org; i.mitsyanko@gmail.com; peter.maydell@linaro.org
>>>>> Cc: qemu-trivial@nongnu.org; Zhanghailiang
>>>>> <zhang.zhanghailiang@huawei.com>
>>>>> Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>>>>> exynos4210_uart_init
>>>>>
>>>>> On 2/12/20 4:36 AM, kuhn.chenqun@huawei.com wrote:
>>>>>> From: Chen Qun <kuhn.chenqun@huawei.com>
>>>>>>
>>>>>> It's easy to reproduce as follow:
>>>>>> virsh qemu-monitor-command vm1 --pretty '{"execute":
>>>>>> "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
>>>>>>
>>>>>> ASAN shows memory leak stack:
>>>>>> #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>>>>>> #2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
>>>>>> #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>>>>>> #4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
>>>>>> #5 0xaaad270beee3 in exynos4210_uart_init
>>>>> /qemu/hw/char/exynos4210_uart.c:677
>>>>>> #6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
>>>>>> #7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
>>>>>> #8 0xaaad2755df2f in qmp_device_list_properties
>>>>>> /qemu/qom/qom-qmp-cmds.c:152
>>>>>>
>>>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>>>> ---
>>>>>> hw/char/exynos4210_uart.c | 8 ++++----
>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>>>>>> index 25d6588e41..5048db5410 100644
>>>>>> --- a/hw/char/exynos4210_uart.c
>>>>>> +++ b/hw/char/exynos4210_uart.c
>>>>>> @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>>>>>> SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>>>>>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>>>>>
>>>>>> - s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>>>> - exynos4210_uart_timeout_int, s);
>>>>>> - s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>>>>>
>>>>> Why are you moving s->wordtime from init() to realize()?
>>>>>
>>>> Hi Philippe, thanks for your reply!
>>>>
>>>> Because I found the variable wordtime is usually used with fifo_timeout_timer.
>>>> Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
>>>>
>>>> I didn't find anything wrong with wordtime in the realize().
>>>> Does it have any other effects?
>>>
>>> IIUC when we use both init() and realize(), realize() should only contains
>>> on code that consumes the object properties... But maybe the design is not
>>> clear. Then why not move all the init() code to realize()?
>>
>> Normally I would recommend the opposite: delay as much as
>> possible to realize(), to avoid unwanted side effects when (e.g.)
>> running qom-list-properties.
>
> Sadly, our documentation on device initialization and realization is
> rather sparse, and developers are left guessing. Their guesses are
> often based on what existing code does. Some of the existing code even
> gets things right.
>
> A few rules from the top of my head:
Worth a new thread...
>
> * Creating and immediately destroying an object must be safe and free of
> side effects: initialization may only touch the object itself, and
> finalization must clean up everything initialization creates.
>
> * unrealize() must clean up everything realize() creates.
Hmm I guess remember someone once said "only for hot-pluggable objects,
else don't bother". But then we make a non-hot-pluggable object as
hot-pluggable and have to fix leaks. Or we start a new hot-pluggable
device based on some code without unrealize()...
>
> * Since initialization cannot fail, code that needs to fail gracefully
> must live in realize().
>
> * Since property values get set between initialization and realization,
> code that uses property values must live in realize().
>
> * Dynamic properties have to be created in initialization to be visible
> in introspection.
>
>> But as s->wordtime is a simple struct field (that we could even
>> decide to expose to the outside as a read-only QOM property), it
>> doesn't really matter. Personally, I would keep it where it is
>> just to avoid churn.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
2020-02-13 16:32 ` Philippe Mathieu-Daudé
@ 2020-02-13 16:37 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2020-02-13 16:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Zhanghailiang, i.mitsyanko, qemu-devel, Markus Armbruster,
qemu-trivial, Chenqun (kuhn),
Eduardo Habkost
On Thu, 13 Feb 2020 at 16:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > * unrealize() must clean up everything realize() creates.
>
> Hmm I guess remember someone once said "only for hot-pluggable objects,
> else don't bother". But then we make a non-hot-pluggable object as
> hot-pluggable and have to fix leaks. Or we start a new hot-pluggable
> device based on some code without unrealize()...
Yeah. Almost all our devices are not hot-pluggable and don't
have unrealize code. Better to just have them stay that way,
or to add untested unreachable code in an unrealize method? Dunno.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-13 16:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 3:36 [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init kuhn.chenqun
2020-02-12 6:16 ` Philippe Mathieu-Daudé
2020-02-12 6:44 ` Chenqun (kuhn)
2020-02-12 7:39 ` Philippe Mathieu-Daudé
2020-02-12 16:19 ` Eduardo Habkost
2020-02-13 2:10 ` Chenqun (kuhn)
2020-02-13 14:28 ` Markus Armbruster
2020-02-13 16:32 ` Philippe Mathieu-Daudé
2020-02-13 16:37 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).