Re: [PATCH] nubus: fix memory leak in nubus_bridge_init()

[Mark Cave-Ayland]

On 02/04/2022 16:47, Laurent Vivier wrote:

Move qbus_init() to a realize function.
Leak detected by valgrind:

   QTEST_QEMU_BINARY="valgrind --leak-check=full \
   --show-leak-kinds=definite ./qemu-system-m68k" \
   tests/qtest/device-introspect-test \
   -p /m68k/device/introspect/concrete/defaults/none
   ...
   ==606164== 6 bytes in 1 blocks are definitely lost in loss record 123 of 
1,771
   ==606164==    at 0x484486F: malloc (vg_replace_malloc.c:381)
   ==606164==    by 0x4FF0428: g_malloc (gmem.c:106)
   ==606164==    by 0x5004433: g_strdup (gstrfuncs.c:364)
   ==606164==    by 0x3D1BA6: memory_region_do_init (memory.c:1170)
   ==606164==    by 0x3D1BA6: memory_region_init (memory.c:1195)
   ==606164==    by 0x375C88: nubus_init (nubus-bus.c:104)
   ==606164==    by 0x49DE82: object_init_with_type (object.c:377)
   ==606164==    by 0x49DE82: object_initialize_with_type (object.c:519)
   ==606164==    by 0x495772: qbus_init (bus.c:158)
   ==606164==    by 0x375DDB: nubus_bridge_init (nubus-bridge.c:21)
   ==606164==    by 0x49DE82: object_init_with_type (object.c:377)
   ==606164==    by 0x49DE82: object_initialize_with_type (object.c:519)
   ==606164==    by 0x49E028: object_new_with_type (object.c:734)
   ==606164==    by 0x5660AC: qmp_device_list_properties (qom-qmp-cmds.c:146)
   ==606164==    by 0x67CC35: qmp_marshal_device_list_properties 
(qapi-commands-qdev.c:66)
   ...

Reported-by: Thomas Huth <th...@redhat.com>
Signed-off-by: Laurent Vivier <laur...@vivier.eu>
---
  hw/nubus/nubus-bridge.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/nubus/nubus-bridge.c b/hw/nubus/nubus-bridge.c
index a42c86080f25..51c3f6328784 100644
--- a/hw/nubus/nubus-bridge.c
+++ b/hw/nubus/nubus-bridge.c
@@ -18,11 +18,17 @@ static void nubus_bridge_init(Object *obj)
      NubusBridge *s = NUBUS_BRIDGE(obj);
      NubusBus *bus = &s->bus;
  
-    qbus_init(bus, sizeof(s->bus), TYPE_NUBUS_BUS, DEVICE(s), NULL);
-
      qdev_init_gpio_out(DEVICE(s), bus->irqs, NUBUS_IRQS);
  }
  
+static void nubus_bridge_realize(DeviceState *d, Error **errp)
+{
+    NubusBridge *s = NUBUS_BRIDGE(d);
+    NubusBus *bus = &s->bus;
+
+    qbus_init(bus, sizeof(NubusBus), TYPE_NUBUS_BUS, d, NULL);
+}
+
  static Property nubus_bridge_properties[] = {
      DEFINE_PROP_UINT16("slot-available-mask", NubusBridge,
                         bus.slot_available_mask, 0xffff),
@@ -34,6 +40,7 @@ static void nubus_bridge_class_init(ObjectClass *klass, void 
*data)
      DeviceClass *dc = DEVICE_CLASS(klass);
  
      dc->fw_name = "nubus";
+    dc->realize = nubus_bridge_realize;
      device_class_set_props(dc, nubus_bridge_properties);
  }

Since qbus_init() is adding a bus that can have child devices, I think the correct 
solution here would be to add an instance_finalize function to the Nubus bridge 
device that calls qbus_finalize().


ATB,

Mark.