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:phi...@redhat.com] > > > Sent: Wednesday, February 12, 2020 2:16 PM > > > To: Chenqun (kuhn) <kuhn.chen...@huawei.com>; qemu- > > > de...@nongnu.org; i.mitsya...@gmail.com; peter.mayd...@linaro.org > > > Cc: qemu-triv...@nongnu.org; Zhanghailiang > > > <zhang.zhanghaili...@huawei.com> > > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in > > > exynos4210_uart_init > > > > > > On 2/12/20 4:36 AM, kuhn.chen...@huawei.com wrote: > > > > From: Chen Qun <kuhn.chen...@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.ro...@huawei.com> > > > > Signed-off-by: Chen Qun <kuhn.chen...@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
