>-----Original Message----- >From: Eduardo Habkost [mailto:ehabk...@redhat.com] >Sent: Thursday, February 13, 2020 12:20 AM >To: Philippe Mathieu-Daudé <phi...@redhat.com> >Cc: Chenqun (kuhn) <kuhn.chen...@huawei.com>; qemu- >de...@nongnu.org; i.mitsya...@gmail.com; peter.mayd...@linaro.org; >qemu-triv...@nongnu.org; Zhanghailiang ><zhang.zhanghaili...@huawei.com>; Markus Armbruster ><arm...@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: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. > OK, Let's keep s->wordtime in init(). I will change it in next version.
Thanks. >-- >Eduardo