| Message ID | 1432814932-12608-2-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote: > From: Shannon Zhao <shannon.zhao@linaro.org> > > valgrind complains about: > ==23381== 8 bytes in 1 blocks are definitely lost in loss record 438 of 2,785 > ==23381== at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) > ==23381== by 0x35478F: malloc_and_trace (vl.c:2556) > ==23381== by 0x64C770E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3) > ==23381== by 0x3ED98B: qemu_extend_irqs (irq.c:55) > ==23381== by 0x3EDA17: qemu_allocate_irqs (irq.c:64) > ==23381== by 0x43D767: ahci_init (ahci.c:1356) > ==23381== by 0x43DAC5: sysbus_ahci_realize (ahci.c:1520) > ==23381== by 0x3EA4CC: device_set_realized (qdev.c:1058) > ==23381== by 0x516CD2: property_set_bool (object.c:1514) > ==23381== by 0x5155CC: object_property_set (object.c:837) > ==23381== by 0x5178EE: object_property_set_qobject (qom-qobject.c:24) > ==23381== by 0x51583B: object_property_set_bool (object.c:905) > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > hw/ide/ahci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 9e5d862..4702a05 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) > ad->port.dma->ops = &ahci_dma_ops; > ide_register_restart_cb(&ad->port); > } > + g_free(irqs); > } Freeing the return value of qemu_allocate_irqs() directly rather than using qemu_free_irqs() looks dubious to me. -- PMM
On 28 May 2015 at 13:24, Shannon Zhao <zhaoshenglong@huawei.com> wrote: > > > On 2015/5/28 20:16, Peter Maydell wrote: >> On 28 May 2015 at 13:08, Shannon Zhao <zhaoshenglong@huawei.com> wrote: >>> > From: Shannon Zhao <shannon.zhao@linaro.org> >>> > --- a/hw/ide/ahci.c >>> > +++ b/hw/ide/ahci.c >>> > @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) >>> > ad->port.dma->ops = &ahci_dma_ops; >>> > ide_register_restart_cb(&ad->port); >>> > } >>> > + g_free(irqs); >>> > } >> Freeing the return value of qemu_allocate_irqs() >> directly rather than using qemu_free_irqs() looks >> dubious to me. > > qemu_free_irqs() will free the return value self of qemu_allocate_irqs() > and also will free all the contents of this array while some of them are > still in use. Yes, but you're still looking into the internal implementation details of this API, which is what I'm suggesting is dubious. For instance, in this case, why are we calling qemu_allocate_irqs() in the first place, rather than just calling qemu_allocate_irq() inside the for() loop in ahci_init()? If code wants to deal with the array of irqs as an array, it should probably be saving the pointer somewhere in its state struct and freeing the whole thing on deinit. If it doesn't actually want an array of irqs (as here) it should probably not be calling qemu_allocate_irqs() in the first place. thanks -- PMM
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9e5d862..4702a05 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1367,6 +1367,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) ad->port.dma->ops = &ahci_dma_ops; ide_register_restart_cb(&ad->port); } + g_free(irqs); } void ahci_uninit(AHCIState *s)