[PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.

[PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.

[Pan Nengyuan]

This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'.
And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, It's not valid in mos6522
if we move timer_new from init to realize, because it's never called at all. So we also add calls to mos6522_realize()
in mac_via_realize to make this move to be valid.

v1:
   - Delay timer_new() from init() to realize() to fix memleaks.
v2:
   - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
   - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1.
v3:
   - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all.
     Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
   - split patch by device to make it more clear.

Pan Nengyuan (6):
  s390x: fix memleaks in cpu_finalize
  hw/arm/pxa2xx: move timer_new from init() into realize() to avoid
    memleaks
  hw/arm/spitz: move timer_new from init() into realize() to avoid
    memleaks
  hw/arm/strongarm: move timer_new from init() into realize() to avoid
    memleaks
  hw/misc/mos6522: move timer_new from init() into realize() to avoid
    memleaks
  hw/timer/cadence_ttc: move timer_new from init() into realize() to
    avoid memleaks

 hw/arm/pxa2xx.c        | 17 +++++++++++------
 hw/arm/spitz.c         |  8 +++++++-
 hw/arm/strongarm.c     | 18 ++++++++++++------
 hw/misc/mac_via.c      |  5 +++++
 hw/misc/mos6522.c      |  6 ++++++
 hw/timer/cadence_ttc.c | 16 +++++++++++-----
 target/s390x/cpu.c     | 22 ++++++++++++++++++----
 7 files changed, 70 insertions(+), 22 deletions(-)

-- 
2.18.2

[PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="base64", Size: 3822 bytes --]

This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
    #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
    #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
    #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
    #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
    #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
    #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
    #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
    #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
    #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
    #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
    #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
    #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Richard Henderson <rth@twiddle.net>
Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
---
v2->v1:
- Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
v3->v2:
- Also do the timer_free in unrealize, it seems more balance.
---
 target/s390x/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index cf84d307c6..cc63c9db22 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
 #if !defined(CONFIG_USER_ONLY)
     S390CPU *cpu = S390_CPU(dev);
+    cpu->env.tod_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
+    cpu->env.cpu_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
 #endif
+
     Error *err = NULL;
 
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
@@ -227,6 +232,18 @@ out:
     error_propagate(errp, err);
 }
 
+static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+#if !defined(CONFIG_USER_ONLY)
+    S390CPU *cpu = S390_CPU(dev);
+
+    timer_del(cpu->env.tod_timer);
+    timer_del(cpu->env.cpu_timer);
+    timer_free(cpu->env.tod_timer);
+    timer_free(cpu->env.cpu_timer);
+#endif
+}
+
 static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
 {
     GuestPanicInformation *panic_info;
@@ -279,10 +296,6 @@ static void s390_cpu_initfn(Object *obj)
                         s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
-    cpu->env.tod_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
-    cpu->env.cpu_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 #endif
 }
@@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     device_class_set_parent_realize(dc, s390_cpu_realizefn,
                                     &scc->parent_realize);
+    dc->unrealize = s390_cpu_unrealizefn;
     device_class_set_props(dc, s390x_cpu_properties);
     dc->user_creatable = true;
 
-- 
2.18.2

[PATCH v3 2/6] hw/arm/pxa2xx: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Andrzej Zaborowski <balrogg@gmail.com>
---
 hw/arm/pxa2xx.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index b33f8f1351..56a36202d7 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1134,18 +1134,22 @@ static void pxa2xx_rtc_init(Object *obj)
     s->last_rtcpicr = 0;
     s->last_hz = s->last_sw = s->last_pi = qemu_clock_get_ms(rtc_clock);
 
+    sysbus_init_irq(dev, &s->rtc_irq);
+
+    memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
+                          "pxa2xx-rtc", 0x10000);
+    sysbus_init_mmio(dev, &s->iomem);
+}
+
+static void pxa2xx_rtc_realize(DeviceState *dev, Error **errp)
+{
+    PXA2xxRTCState *s = PXA2XX_RTC(dev);
     s->rtc_hz    = timer_new_ms(rtc_clock, pxa2xx_rtc_hz_tick,    s);
     s->rtc_rdal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal1_tick, s);
     s->rtc_rdal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal2_tick, s);
     s->rtc_swal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal1_tick, s);
     s->rtc_swal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal2_tick, s);
     s->rtc_pi    = timer_new_ms(rtc_clock, pxa2xx_rtc_pi_tick,    s);
-
-    sysbus_init_irq(dev, &s->rtc_irq);
-
-    memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
-                          "pxa2xx-rtc", 0x10000);
-    sysbus_init_mmio(dev, &s->iomem);
 }
 
 static int pxa2xx_rtc_pre_save(void *opaque)
@@ -1203,6 +1207,7 @@ static void pxa2xx_rtc_sysbus_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "PXA2xx RTC Controller";
     dc->vmsd = &vmstate_pxa2xx_rtc_regs;
+    dc->realize = pxa2xx_rtc_realize;
 }
 
 static const TypeInfo pxa2xx_rtc_sysbus_info = {
-- 
2.18.2

[PATCH v3 3/6] hw/arm/spitz: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Andrzej Zaborowski <balrogg@gmail.com>
---
 hw/arm/spitz.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index e001088103..cbfa6934cf 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -524,11 +524,16 @@ static void spitz_keyboard_init(Object *obj)
 
     spitz_keyboard_pre_map(s);
 
-    s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
     qdev_init_gpio_in(dev, spitz_keyboard_strobe, SPITZ_KEY_STROBE_NUM);
     qdev_init_gpio_out(dev, s->sense, SPITZ_KEY_SENSE_NUM);
 }
 
+static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
+{
+    SpitzKeyboardState *s = SPITZ_KEYBOARD(dev);
+    s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
+}
+
 /* LCD backlight controller */
 
 #define LCDTG_RESCTL	0x00
@@ -1115,6 +1120,7 @@ static void spitz_keyboard_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_spitz_kbd;
+    dc->realize = spitz_keyboard_realize;
 }
 
 static const TypeInfo spitz_keyboard_info = {
-- 
2.18.2

[PATCH v3 4/6] hw/arm/strongarm: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
 hw/arm/strongarm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index cd8a99aaf2..3010d765bb 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -399,9 +399,6 @@ static void strongarm_rtc_init(Object *obj)
     s->last_rcnr = (uint32_t) mktimegm(&tm);
     s->last_hz = qemu_clock_get_ms(rtc_clock);
 
-    s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
-    s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
-
     sysbus_init_irq(dev, &s->rtc_irq);
     sysbus_init_irq(dev, &s->rtc_hz_irq);
 
@@ -410,6 +407,13 @@ static void strongarm_rtc_init(Object *obj)
     sysbus_init_mmio(dev, &s->iomem);
 }
 
+static void strongarm_rtc_realize(DeviceState *dev, Error **errp)
+{
+    StrongARMRTCState *s = STRONGARM_RTC(dev);
+    s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
+    s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
+}
+
 static int strongarm_rtc_pre_save(void *opaque)
 {
     StrongARMRTCState *s = opaque;
@@ -451,6 +455,7 @@ static void strongarm_rtc_sysbus_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "StrongARM RTC Controller";
     dc->vmsd = &vmstate_strongarm_rtc_regs;
+    dc->realize = strongarm_rtc_realize;
 }
 
 static const TypeInfo strongarm_rtc_sysbus_info = {
@@ -1240,15 +1245,16 @@ static void strongarm_uart_init(Object *obj)
                           "uart", 0x10000);
     sysbus_init_mmio(dev, &s->iomem);
     sysbus_init_irq(dev, &s->irq);
-
-    s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_rx_to, s);
-    s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
 }
 
 static void strongarm_uart_realize(DeviceState *dev, Error **errp)
 {
     StrongARMUARTState *s = STRONGARM_UART(dev);
 
+    s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                       strongarm_uart_rx_to,
+                                       s);
+    s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
     qemu_chr_fe_set_handlers(&s->chr,
                              strongarm_uart_can_receive,
                              strongarm_uart_receive,
-- 
2.18.2

[PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Laurent Vivier <laurent@vivier.eu>
---
v2->v1:
- no changes in this patch.
v3->v2:
- remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
---
 hw/misc/mac_via.c | 5 +++++
 hw/misc/mos6522.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..1d72d4ef35 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
     sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
                           sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
 
+    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
+                             &error_abort);
+    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
+                             &error_abort);
+
     /* Pass through mos6522 output IRQs */
     ms = MOS6522(&m->mos6522_via1);
     object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..c1cd154a84 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
     for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
         s->timers[i].index = i;
     }
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+    MOS6522State *s = MOS6522(dev);
 
     s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
     s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
 
     dc->reset = mos6522_reset;
     dc->vmsd = &vmstate_mos6522;
+    dc->realize = mos6522_realize;
     device_class_set_props(dc, mos6522_properties);
     mdc->parent_reset = dc->reset;
     mdc->set_sr_int = mos6522_set_sr_int;
-- 
2.18.2

[PATCH v3 6/6] hw/timer/cadence_ttc: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
---
 hw/timer/cadence_ttc.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
index 5e3128c1e3..b0ba6b2bba 100644
--- a/hw/timer/cadence_ttc.c
+++ b/hw/timer/cadence_ttc.c
@@ -412,16 +412,21 @@ static void cadence_timer_init(uint32_t freq, CadenceTimerState *s)
 static void cadence_ttc_init(Object *obj)
 {
     CadenceTTCState *s = CADENCE_TTC(obj);
+
+    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
+                          "timer", 0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static void cadence_ttc_realize(DeviceState *dev, Error **errp)
+{
+    CadenceTTCState *s = CADENCE_TTC(dev);
     int i;
 
     for (i = 0; i < 3; ++i) {
         cadence_timer_init(133000000, &s->timer[i]);
-        sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->timer[i].irq);
+        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->timer[i].irq);
     }
-
-    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
-                          "timer", 0x1000);
-    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
 }
 
 static int cadence_timer_pre_save(void *opaque)
@@ -479,6 +484,7 @@ static void cadence_ttc_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_cadence_ttc;
+    dc->realize = cadence_ttc_realize;
 }
 
 static const TypeInfo cadence_ttc_info = {
-- 
2.18.2

Re: [PATCH v3 6/6] hw/timer/cadence_ttc: move timer_new from init() into realize() to avoid memleaks

[Alistair Francis]

On Wed, Feb 26, 2020 at 6:37 PM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> ---
>  hw/timer/cadence_ttc.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
> index 5e3128c1e3..b0ba6b2bba 100644
> --- a/hw/timer/cadence_ttc.c
> +++ b/hw/timer/cadence_ttc.c
> @@ -412,16 +412,21 @@ static void cadence_timer_init(uint32_t freq, CadenceTimerState *s)
>  static void cadence_ttc_init(Object *obj)
>  {
>      CadenceTTCState *s = CADENCE_TTC(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
> +                          "timer", 0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +}
> +
> +static void cadence_ttc_realize(DeviceState *dev, Error **errp)
> +{
> +    CadenceTTCState *s = CADENCE_TTC(dev);
>      int i;
>
>      for (i = 0; i < 3; ++i) {
>          cadence_timer_init(133000000, &s->timer[i]);
> -        sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->timer[i].irq);
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->timer[i].irq);
>      }
> -
> -    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
> -                          "timer", 0x1000);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>  }
>
>  static int cadence_timer_pre_save(void *opaque)
> @@ -479,6 +484,7 @@ static void cadence_ttc_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->vmsd = &vmstate_cadence_ttc;
> +    dc->realize = cadence_ttc_realize;
>  }
>
>  static const TypeInfo cadence_ttc_info = {
> --
> 2.18.2
>
>

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[David Hildenbrand]

On 27.02.20 03:50, Pan Nengyuan wrote:
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
> v3->v2:
> - Also do the timer_free in unrealize, it seems more balance.
> ---


As I already said, I think this is init and not realize stuff. Do we
have a convention now and documented that?

Anyhow, I don't really care
[...]


> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>  
>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>                                      &scc->parent_realize);
> +    dc->unrealize = s390_cpu_unrealizefn;

Shouldn't we use device_class_set_parent_unrealize?


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Philippe Mathieu-Daudé]

On 2/27/20 9:41 AM, David Hildenbrand wrote:
> On 27.02.20 03:50, Pan Nengyuan wrote:
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>      #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>      #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>      #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>      #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>      #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>      #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>      #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>      #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>      #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>      #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>      #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>      #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>      #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
> 
> 
> As I already said, I think this is init and not realize stuff. Do we
> have a convention now and documented that?

The clearer doc I read so far is this post:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg680187.html
(but see the thread for more helpful comments)

Another thread that you might find interesting is "how to handle QOM 
'container' objects whose contents depend on QOM properties?"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg511703.html

> 
> Anyhow, I don't really care
> [...]

Well, looking at the time spent on these series and their review, having 
it better documented might save time the whole community.

> 
>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>   
>>       device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>                                       &scc->parent_realize);
>> +    dc->unrealize = s390_cpu_unrealizefn;
> 
> Shouldn't we use device_class_set_parent_unrealize?
> 
> 

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

On 2/27/2020 4:41 PM, David Hildenbrand wrote:
> On 27.02.20 03:50, Pan Nengyuan wrote:
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
> 
> 
> As I already said, I think this is init and not realize stuff. Do we
> have a convention now and documented that?
> 
> Anyhow, I don't really care
> [...]
> 
> 
>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>  
>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>                                      &scc->parent_realize);
>> +    dc->unrealize = s390_cpu_unrealizefn;
> 
> Shouldn't we use device_class_set_parent_unrealize?

We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.

typedef struct S390CPUClass {
...
DeviceRealize parent_realize;    // no parent_unrealize;
...
}

So I think we can't use it.

Thanks.

> 
> 

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[David Hildenbrand]

On 27.02.20 09:58, Pan Nengyuan wrote:
> 
> 
> On 2/27/2020 4:41 PM, David Hildenbrand wrote:
>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>
>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>> Cc: qemu-s390x@nongnu.org
>>> ---
>>> v2->v1:
>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>> v3->v2:
>>> - Also do the timer_free in unrealize, it seems more balance.
>>> ---
>>
>>
>> As I already said, I think this is init and not realize stuff. Do we
>> have a convention now and documented that?
>>
>> Anyhow, I don't really care
>> [...]
>>
>>
>>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>>  
>>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>>                                      &scc->parent_realize);
>>> +    dc->unrealize = s390_cpu_unrealizefn;
>>
>> Shouldn't we use device_class_set_parent_unrealize?
> 
> We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.
> 
> typedef struct S390CPUClass {
> ...
> DeviceRealize parent_realize;    // no parent_unrealize;
> ...
> }
> 
> So I think we can't use it.

So you should add it and properly call the parent_unrealize from your
new unrealize function?

AFAIKS you are overwriting cpu_common_unrealizefn set in hw/core/cpu.c
for TYPE_CPU with this change.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[David Hildenbrand]

On 27.02.20 09:55, Philippe Mathieu-Daudé wrote:
> On 2/27/20 9:41 AM, David Hildenbrand wrote:
>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>
>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>      #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>      #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>      #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>      #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>      #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>      #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>      #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>      #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>      #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>      #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>      #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>      #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>      #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>> Cc: qemu-s390x@nongnu.org
>>> ---
>>> v2->v1:
>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>> v3->v2:
>>> - Also do the timer_free in unrealize, it seems more balance.
>>> ---
>>
>>
>> As I already said, I think this is init and not realize stuff. Do we
>> have a convention now and documented that?
> 
> The clearer doc I read so far is this post:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg680187.html
> (but see the thread for more helpful comments)
> 
> Another thread that you might find interesting is "how to handle QOM 
> 'container' objects whose contents depend on QOM properties?"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg511703.html
> 
>>
>> Anyhow, I don't really care
>> [...]
> 
> Well, looking at the time spent on these series and their review, having 
> it better documented might save time the whole community.

Thanks for the pointers. Yes, we should document that. Especially if it
might save me some time ;)

Moving stuff around without a clear convention is not-so-nice IMHO.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

On 2/27/2020 5:04 PM, David Hildenbrand wrote:
> On 27.02.20 09:58, Pan Nengyuan wrote:
>>
>>
>> On 2/27/2020 4:41 PM, David Hildenbrand wrote:
>>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>>
>>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>>> ---
>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>>> Cc: qemu-s390x@nongnu.org
>>>> ---
>>>> v2->v1:
>>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>>> v3->v2:
>>>> - Also do the timer_free in unrealize, it seems more balance.
>>>> ---
>>>
>>>
>>> As I already said, I think this is init and not realize stuff. Do we
>>> have a convention now and documented that?
>>>
>>> Anyhow, I don't really care
>>> [...]
>>>
>>>
>>>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>>>  
>>>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>>>                                      &scc->parent_realize);
>>>> +    dc->unrealize = s390_cpu_unrealizefn;
>>>
>>> Shouldn't we use device_class_set_parent_unrealize?
>>
>> We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.
>>
>> typedef struct S390CPUClass {
>> ...
>> DeviceRealize parent_realize;    // no parent_unrealize;
>> ...
>> }
>>
>> So I think we can't use it.
> 
> So you should add it and properly call the parent_unrealize from your
> new unrealize function?
> 
> AFAIKS you are overwriting cpu_common_unrealizefn set in hw/core/cpu.c
> for TYPE_CPU with this change.

Oh, I think you are right, I will change it.

Thanks.
> 

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Cornelia Huck]

On Thu, 27 Feb 2020 10:50:50 +0800
Pan Nengyuan <pannengyuan@huawei.com> wrote:

> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
> v3->v2:
> - Also do the timer_free in unrealize, it seems more balance.
> ---
>  target/s390x/cpu.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index cf84d307c6..cc63c9db22 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>  #if !defined(CONFIG_USER_ONLY)
>      S390CPU *cpu = S390_CPU(dev);
> +    cpu->env.tod_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> +    cpu->env.cpu_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>  #endif

It does not seem you addressed the comments I had last time, namely
- memory leak on error (we do not go through unrealize if the device
  was never completely realized)
- coding style (initialization in middle of declaration section)

> +
>      Error *err = NULL;
>  
>      /* the model has to be realized before qemu_init_vcpu() due to kvm */

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

On 2/27/2020 7:06 PM, Cornelia Huck wrote:
> On Thu, 27 Feb 2020 10:50:50 +0800
> Pan Nengyuan <pannengyuan@huawei.com> wrote:
> 
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
>>  target/s390x/cpu.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index cf84d307c6..cc63c9db22 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>>  #if !defined(CONFIG_USER_ONLY)
>>      S390CPU *cpu = S390_CPU(dev);
>> +    cpu->env.tod_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>> +    cpu->env.cpu_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>>  #endif
> 
> It does not seem you addressed the comments I had last time, namely
> - memory leak on error (we do not go through unrealize if the device
>   was never completely realized)
> - coding style (initialization in middle of declaration section)

I am sorry, I misread the peter's reply and miss the codeing style too.

Apologies for you. I will change it.

Thanks.

> 
>> +
>>      Error *err = NULL;
>>  
>>      /* the model has to be realized before qemu_init_vcpu() due to kvm */
> 
> .
> 

Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Peter Maydell]

On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Laurent Vivier <laurent@vivier.eu>
> ---
> v2->v1:
> - no changes in this patch.
> v3->v2:
> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.

Hi; this is really fixing two bugs in one patch:

> ---
>  hw/misc/mac_via.c | 5 +++++
>  hw/misc/mos6522.c | 6 ++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..1d72d4ef35 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>      sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>
> +    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
> +                             &error_abort);
> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
> +                             &error_abort);
> +
>      /* Pass through mos6522 output IRQs */
>      ms = MOS6522(&m->mos6522_via1);
>      object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),

This is fixing a bug in mac_via where it failed to actually
realize devices it was using. That's a dependency for the bug
you're trying to fix, but it's a separate one and should be
in its own patch.

You also want to pass the error back up to the caller, rather
than using error_abort. To do that, at the top of the function:

    Error *err = NULL;

and then here you can do:
    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

The existing code which inits the mos6522 objects and
calls object_property_add_alias on them which is in
the mac_via realize function should be moved to the init
function. (init should init child objects and set up
properties; realize should realize them.)

This is all fixing the incorrect creation of the mos6522
devices in mac_via (which has been wrong since the mac_via
was first added in commit 6dca62a0000f9), so that can all
be one patch I think.

> +static void mos6522_realize(DeviceState *dev, Error **errp)
> +{
> +    MOS6522State *s = MOS6522(dev);
>
>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>
>      dc->reset = mos6522_reset;
>      dc->vmsd = &vmstate_mos6522;
> +    dc->realize = mos6522_realize;
>      device_class_set_props(dc, mos6522_properties);
>      mdc->parent_reset = dc->reset;
>      mdc->set_sr_int = mos6522_set_sr_int;

The changes to hw/misc/mos_6522.c are good, but should be in
their own patch.

thanks
-- PMM

Re: [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.

[Peter Maydell]

On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'.
> And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, It's not valid in mos6522
> if we move timer_new from init to realize, because it's never called at all. So we also add calls to mos6522_realize()
> in mac_via_realize to make this move to be valid.
>
> v1:
>    - Delay timer_new() from init() to realize() to fix memleaks.
> v2:
>    - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
>    - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1.
> v3:
>    - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all.
>      Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>    - split patch by device to make it more clear.

Hi; I've applied patches 2, 3, 4 and 6 to target-arm.next,
since I think those ones are OK and they're all arm related.

You've already got review comment for patch 1 (s390)
and 5 (m68k mac_via/mos6522).

thanks
-- PMM

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Stefan Hajnoczi]

On Thu, Feb 27, 2020 at 2:42 AM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)

Hi,
This email is invalid and cannot be parsed by the patches
(https://github.com/stefanha/patches) tool that is used by some QEMU
maintainers to apply patches.

The character set is incorrectly set to "base64", which is a content
transfer encoding and not a character set:

  Content-Type: text/plain; charset="base64"
  Content-Transfer-Encoding: quoted-printable

There is a UTF-8 é character here:

  - Similarly to other cleanups, move timer_new into realize(Suggested by Phi=
  lippe Mathieu-Daud=C3=A9)

Since there is no valid charset the é character cannot be decoded.

This might be a mail server problem but it could also be due to your
git-send-email(1) configuration.

Did you set the charset to "base64" or override the content transfer
encoding?  I think other people on the list will have trouble
receiving emails like this too.

Stefan

Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Mark Cave-Ayland]

On 02/03/2020 13:21, Peter Maydell wrote:

> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> ---
>> v2->v1:
>> - no changes in this patch.
>> v3->v2:
>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
> 
> Hi; this is really fixing two bugs in one patch:
> 
>> ---
>>  hw/misc/mac_via.c | 5 +++++
>>  hw/misc/mos6522.c | 6 ++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..1d72d4ef35 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>>      sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>
>> +    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>> +                             &error_abort);
>> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>> +                             &error_abort);
>> +
>>      /* Pass through mos6522 output IRQs */
>>      ms = MOS6522(&m->mos6522_via1);
>>      object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> 
> This is fixing a bug in mac_via where it failed to actually
> realize devices it was using. That's a dependency for the bug
> you're trying to fix, but it's a separate one and should be
> in its own patch.

Sigh. Thanks for this - I actually discovered this a little while back and have some
local patches to do the same, but due to lack of time I never managed to tidy them up
for submission.

> You also want to pass the error back up to the caller, rather
> than using error_abort. To do that, at the top of the function:
> 
>     Error *err = NULL;
> 
> and then here you can do:
>     object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
>     if (err) {
>         error_propagate(errp, err);
>         return;
>     }
> 
> The existing code which inits the mos6522 objects and
> calls object_property_add_alias on them which is in
> the mac_via realize function should be moved to the init
> function. (init should init child objects and set up
> properties; realize should realize them.)

And I believe at some point I had a patch lying around to do this too...

> This is all fixing the incorrect creation of the mos6522
> devices in mac_via (which has been wrong since the mac_via
> was first added in commit 6dca62a0000f9), so that can all
> be one patch I think.
> 
>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>> +{
>> +    MOS6522State *s = MOS6522(dev);
>>
>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>>
>>      dc->reset = mos6522_reset;
>>      dc->vmsd = &vmstate_mos6522;
>> +    dc->realize = mos6522_realize;
>>      device_class_set_props(dc, mos6522_properties);
>>      mdc->parent_reset = dc->reset;
>>      mdc->set_sr_int = mos6522_set_sr_int;
> 
> The changes to hw/misc/mos_6522.c are good, but should be in
> their own patch.


ATB,

Mark.

Re: [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.

[Pan Nengyuan]

On 3/2/2020 9:21 PM, Peter Maydell wrote:
> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'.
>> And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, It's not valid in mos6522
>> if we move timer_new from init to realize, because it's never called at all. So we also add calls to mos6522_realize()
>> in mac_via_realize to make this move to be valid.
>>
>> v1:
>>    - Delay timer_new() from init() to realize() to fix memleaks.
>> v2:
>>    - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
>>    - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1.
>> v3:
>>    - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all.
>>      Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>    - split patch by device to make it more clear.
> 
> Hi; I've applied patches 2, 3, 4 and 6 to target-arm.next,
> since I think those ones are OK and they're all arm related.
> 
> You've already got review comment for patch 1 (s390)
> and 5 (m68k mac_via/mos6522).

Fine, thanks.

> 
> thanks
> -- PMM
> 

Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote:
> On 02/03/2020 13:21, Peter Maydell wrote:
> 
>> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>>
>>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>> v2->v1:
>>> - no changes in this patch.
>>> v3->v2:
>>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>
>> Hi; this is really fixing two bugs in one patch:
>>
>>> ---
>>>  hw/misc/mac_via.c | 5 +++++
>>>  hw/misc/mos6522.c | 6 ++++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index b7d0012794..1d72d4ef35 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>>>      sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>>                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>>
>>> +    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>>> +                             &error_abort);
>>> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>>> +                             &error_abort);
>>> +
>>>      /* Pass through mos6522 output IRQs */
>>>      ms = MOS6522(&m->mos6522_via1);
>>>      object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>
>> This is fixing a bug in mac_via where it failed to actually
>> realize devices it was using. That's a dependency for the bug
>> you're trying to fix, but it's a separate one and should be
>> in its own patch.
> 
> Sigh. Thanks for this - I actually discovered this a little while back and have some
> local patches to do the same, but due to lack of time I never managed to tidy them up
> for submission.

Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together?
Or If you have no time, I can help to do it about this device. :)

> 
>> You also want to pass the error back up to the caller, rather
>> than using error_abort. To do that, at the top of the function:
>>
>>     Error *err = NULL;
>>
>> and then here you can do:
>>     object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
>>     if (err) {
>>         error_propagate(errp, err);
>>         return;
>>     }
>>
>> The existing code which inits the mos6522 objects and
>> calls object_property_add_alias on them which is in
>> the mac_via realize function should be moved to the init
>> function. (init should init child objects and set up
>> properties; realize should realize them.)
> 
> And I believe at some point I had a patch lying around to do this too...
> 
>> This is all fixing the incorrect creation of the mos6522
>> devices in mac_via (which has been wrong since the mac_via
>> was first added in commit 6dca62a0000f9), so that can all
>> be one patch I think.
>>
>>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    MOS6522State *s = MOS6522(dev);
>>>
>>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>>>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
>>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>>>
>>>      dc->reset = mos6522_reset;
>>>      dc->vmsd = &vmstate_mos6522;
>>> +    dc->realize = mos6522_realize;
>>>      device_class_set_props(dc, mos6522_properties);
>>>      mdc->parent_reset = dc->reset;
>>>      mdc->set_sr_int = mos6522_set_sr_int;
>>
>> The changes to hw/misc/mos_6522.c are good, but should be in
>> their own patch.
> 
> 
> ATB,
> 
> Mark.
> .
> 

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

On 3/2/2020 10:34 PM, Stefan Hajnoczi wrote:
> On Thu, Feb 27, 2020 at 2:42 AM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
> 
> Hi,
> This email is invalid and cannot be parsed by the patches
> (https://github.com/stefanha/patches) tool that is used by some QEMU
> maintainers to apply patches.
> 
> The character set is incorrectly set to "base64", which is a content
> transfer encoding and not a character set:
> 
>   Content-Type: text/plain; charset="base64"
>   Content-Transfer-Encoding: quoted-printable
> 
> There is a UTF-8 é character here:
> 
>   - Similarly to other cleanups, move timer_new into realize(Suggested by Phi=
>   lippe Mathieu-Daud=C3=A9)
> 
> Since there is no valid charset the é character cannot be decoded.
> 
> This might be a mail server problem but it could also be due to your
> git-send-email(1) configuration.
> 
> Did you set the charset to "base64" or override the content transfer
> encoding?  I think other people on the list will have trouble
> receiving emails like this too.

Yes, it's set to "base64", I will correct it.

Thanks.

> 
> Stefan
> .
> 

Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Mark Cave-Ayland]

On 03/03/2020 01:36, Pan Nengyuan wrote:

> On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote:
>> On 02/03/2020 13:21, Peter Maydell wrote:
>>
>>> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>>>
>>>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>>>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>>> ---
>>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>> v2->v1:
>>>> - no changes in this patch.
>>>> v3->v2:
>>>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>
>>> Hi; this is really fixing two bugs in one patch:
>>>
>>>> ---
>>>>  hw/misc/mac_via.c | 5 +++++
>>>>  hw/misc/mos6522.c | 6 ++++++
>>>>  2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>> index b7d0012794..1d72d4ef35 100644
>>>> --- a/hw/misc/mac_via.c
>>>> +++ b/hw/misc/mac_via.c
>>>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>>>>      sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>>>                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>>>
>>>> +    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>>>> +                             &error_abort);
>>>> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>>>> +                             &error_abort);
>>>> +
>>>>      /* Pass through mos6522 output IRQs */
>>>>      ms = MOS6522(&m->mos6522_via1);
>>>>      object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>>
>>> This is fixing a bug in mac_via where it failed to actually
>>> realize devices it was using. That's a dependency for the bug
>>> you're trying to fix, but it's a separate one and should be
>>> in its own patch.
>>
>> Sigh. Thanks for this - I actually discovered this a little while back and have some
>> local patches to do the same, but due to lack of time I never managed to tidy them up
>> for submission.
> 
> Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together?
> Or If you have no time, I can help to do it about this device. :)

My patches are part of various q800 branches I have been playing with over the past
couple of months, and so are lagging quite far behind master. If you are able to
update them based upon Peter's comments them I'm happy to review them (and perhaps
could perhaps take them along with my cmd646 patchset if required).


ATB,

Mark.