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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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