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

[Chenqun (kuhn)]

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

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);
>>