RE: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init

[Chenqun (kuhn)]

>-----Original Message-----
>From: Eduardo Habkost [mailto:address@hidden]
>Sent: Thursday, February 13, 2020 12:20 AM
>To: Philippe Mathieu-Daudé <address@hidden>
>Cc: Chenqun (kuhn) <address@hidden>; qemu-
>address@hidden; address@hidden; address@hidden;
>address@hidden; Zhanghailiang
><address@hidden>; Markus Armbruster
><address@hidden>
>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:address@hidden]
>> > > Sent: Wednesday, February 12, 2020 2:16 PM
>> > > To: Chenqun (kuhn) <address@hidden>; qemu-
>> > > address@hidden; address@hidden; address@hidden
>> > > Cc: address@hidden; Zhanghailiang
>> > > <address@hidden>
>> > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>> > > exynos4210_uart_init
>> > >
>> > > On 2/12/20 4:36 AM, address@hidden wrote:
>> > > > From: Chen Qun <address@hidden>
>> > > >
>> > > > 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 <address@hidden>
>> > > > Signed-off-by: Chen Qun <address@hidden>
>> > > > ---
>> > > >    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