* [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() @ 2016-08-16 17:10 Ashijeet Acharya 2016-09-01 5:31 ` Ashijeet Acharya 0 siblings, 1 reply; 8+ messages in thread From: Ashijeet Acharya @ 2016-08-16 17:10 UTC (permalink / raw) To: jsnow; +Cc: pbonzini, qemu-devel, Ashijeet Acharya Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- hw/ide/core.c | 2 +- hw/ide/qdev.c | 14 ++++++++++++++ include/hw/ide/internal.h | 1 + 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 45b6df1..eecbb47 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) void ide_register_restart_cb(IDEBus *bus) { if (bus->dma->ops->restart_dma) { - qemu_add_vm_change_state_handler(ide_restart_cb, bus); + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); } } diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 67c76bf..6f75f77 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -31,6 +31,7 @@ /* --------------------------------- */ static char *idebus_get_fw_dev_path(DeviceState *dev); +static void idebus_unrealize(DeviceState *qdev, Error **errp); static Property ide_props[] = { DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) k->init = ide_qdev_init; set_bit(DEVICE_CATEGORY_STORAGE, k->categories); k->bus_type = TYPE_IDE_BUS; + k->unrealize = idebus_unrealize; k->props = ide_props; } @@ -368,3 +370,15 @@ static void ide_register_types(void) } type_init(ide_register_types) + +static void idebus_unrealize(DeviceState *qdev, Error **errp) +{ + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); + + if (bus->dma->ops->restart_dma) { + if (bus->vmstate) { + qemu_del_vm_change_state_handler(bus->vmstate); + } + } +} diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 7824bc3..2103261 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -480,6 +480,7 @@ struct IDEBus { uint8_t retry_unit; int64_t retry_sector_num; uint32_t retry_nsector; + VMChangeStateEntry *vmstate; }; #define TYPE_IDE_DEVICE "ide-device" -- 2.6.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-08-16 17:10 [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() Ashijeet Acharya @ 2016-09-01 5:31 ` Ashijeet Acharya 2016-09-01 15:43 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Ashijeet Acharya @ 2016-09-01 5:31 UTC (permalink / raw) To: John Snow; +Cc: Paolo Bonzini, QEMU Developers, Ashijeet Acharya I am still waiting for review on this one. On Tue, Aug 16, 2016 at 10:40 PM, Ashijeet Acharya <ashijeetacharya@gmail.com> wrote: > Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > hw/ide/core.c | 2 +- > hw/ide/qdev.c | 14 ++++++++++++++ > include/hw/ide/internal.h | 1 + > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 45b6df1..eecbb47 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) > void ide_register_restart_cb(IDEBus *bus) > { > if (bus->dma->ops->restart_dma) { > - qemu_add_vm_change_state_handler(ide_restart_cb, bus); > + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); > } > } > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index 67c76bf..6f75f77 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -31,6 +31,7 @@ > /* --------------------------------- */ > > static char *idebus_get_fw_dev_path(DeviceState *dev); > +static void idebus_unrealize(DeviceState *qdev, Error **errp); > > static Property ide_props[] = { > DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), > @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) > k->init = ide_qdev_init; > set_bit(DEVICE_CATEGORY_STORAGE, k->categories); > k->bus_type = TYPE_IDE_BUS; > + k->unrealize = idebus_unrealize; > k->props = ide_props; > } > > @@ -368,3 +370,15 @@ static void ide_register_types(void) > } > > type_init(ide_register_types) > + > +static void idebus_unrealize(DeviceState *qdev, Error **errp) > +{ > + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); > + > + if (bus->dma->ops->restart_dma) { > + if (bus->vmstate) { > + qemu_del_vm_change_state_handler(bus->vmstate); > + } > + } > +} > > diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h > index 7824bc3..2103261 100644 > --- a/include/hw/ide/internal.h > +++ b/include/hw/ide/internal.h > @@ -480,6 +480,7 @@ struct IDEBus { > uint8_t retry_unit; > int64_t retry_sector_num; > uint32_t retry_nsector; > + VMChangeStateEntry *vmstate; > }; > > #define TYPE_IDE_DEVICE "ide-device" > -- > 2.6.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-09-01 5:31 ` Ashijeet Acharya @ 2016-09-01 15:43 ` Paolo Bonzini 2016-09-01 15:45 ` Ashijeet Acharya 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-09-01 15:43 UTC (permalink / raw) To: Ashijeet Acharya, John Snow; +Cc: QEMU Developers On 01/09/2016 07:31, Ashijeet Acharya wrote: > I am still waiting for review on this one. Hi, QEMU is in hard freeze now so it's normal to have some delay in patch review. Maintainers often use this time to work on their own features. I'm sure John will get to it in short time. Paolo > On Tue, Aug 16, 2016 at 10:40 PM, Ashijeet Acharya > <ashijeetacharya@gmail.com> wrote: >> Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> hw/ide/core.c | 2 +- >> hw/ide/qdev.c | 14 ++++++++++++++ >> include/hw/ide/internal.h | 1 + >> 3 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index 45b6df1..eecbb47 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) >> void ide_register_restart_cb(IDEBus *bus) >> { >> if (bus->dma->ops->restart_dma) { >> - qemu_add_vm_change_state_handler(ide_restart_cb, bus); >> + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); >> } >> } >> >> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c >> index 67c76bf..6f75f77 100644 >> --- a/hw/ide/qdev.c >> +++ b/hw/ide/qdev.c >> @@ -31,6 +31,7 @@ >> /* --------------------------------- */ >> >> static char *idebus_get_fw_dev_path(DeviceState *dev); >> +static void idebus_unrealize(DeviceState *qdev, Error **errp); >> >> static Property ide_props[] = { >> DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), >> @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) >> k->init = ide_qdev_init; >> set_bit(DEVICE_CATEGORY_STORAGE, k->categories); >> k->bus_type = TYPE_IDE_BUS; >> + k->unrealize = idebus_unrealize; >> k->props = ide_props; >> } >> >> @@ -368,3 +370,15 @@ static void ide_register_types(void) >> } >> >> type_init(ide_register_types) >> + >> +static void idebus_unrealize(DeviceState *qdev, Error **errp) >> +{ >> + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); >> + >> + if (bus->dma->ops->restart_dma) { >> + if (bus->vmstate) { >> + qemu_del_vm_change_state_handler(bus->vmstate); >> + } >> + } >> +} >> >> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h >> index 7824bc3..2103261 100644 >> --- a/include/hw/ide/internal.h >> +++ b/include/hw/ide/internal.h >> @@ -480,6 +480,7 @@ struct IDEBus { >> uint8_t retry_unit; >> int64_t retry_sector_num; >> uint32_t retry_nsector; >> + VMChangeStateEntry *vmstate; >> }; >> >> #define TYPE_IDE_DEVICE "ide-device" >> -- >> 2.6.2 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-09-01 15:43 ` Paolo Bonzini @ 2016-09-01 15:45 ` Ashijeet Acharya 0 siblings, 0 replies; 8+ messages in thread From: Ashijeet Acharya @ 2016-09-01 15:45 UTC (permalink / raw) To: Paolo Bonzini; +Cc: John Snow, QEMU Developers On Thu, Sep 1, 2016 at 9:13 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 01/09/2016 07:31, Ashijeet Acharya wrote: >> I am still waiting for review on this one. > > Hi, > > QEMU is in hard freeze now so it's normal to have some delay in patch > review. Maintainers often use this time to work on their own features. > > I'm sure John will get to it in short time. > > Paolo Alright thanks. No problem! Ashijeet > >> On Tue, Aug 16, 2016 at 10:40 PM, Ashijeet Acharya >> <ashijeetacharya@gmail.com> wrote: >>> Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. >>> >>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >>> --- >>> hw/ide/core.c | 2 +- >>> hw/ide/qdev.c | 14 ++++++++++++++ >>> include/hw/ide/internal.h | 1 + >>> 3 files changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c >>> index 45b6df1..eecbb47 100644 >>> --- a/hw/ide/core.c >>> +++ b/hw/ide/core.c >>> @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) >>> void ide_register_restart_cb(IDEBus *bus) >>> { >>> if (bus->dma->ops->restart_dma) { >>> - qemu_add_vm_change_state_handler(ide_restart_cb, bus); >>> + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); >>> } >>> } >>> >>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c >>> index 67c76bf..6f75f77 100644 >>> --- a/hw/ide/qdev.c >>> +++ b/hw/ide/qdev.c >>> @@ -31,6 +31,7 @@ >>> /* --------------------------------- */ >>> >>> static char *idebus_get_fw_dev_path(DeviceState *dev); >>> +static void idebus_unrealize(DeviceState *qdev, Error **errp); >>> >>> static Property ide_props[] = { >>> DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), >>> @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) >>> k->init = ide_qdev_init; >>> set_bit(DEVICE_CATEGORY_STORAGE, k->categories); >>> k->bus_type = TYPE_IDE_BUS; >>> + k->unrealize = idebus_unrealize; >>> k->props = ide_props; >>> } >>> >>> @@ -368,3 +370,15 @@ static void ide_register_types(void) >>> } >>> >>> type_init(ide_register_types) >>> + >>> +static void idebus_unrealize(DeviceState *qdev, Error **errp) >>> +{ >>> + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); >>> + >>> + if (bus->dma->ops->restart_dma) { >>> + if (bus->vmstate) { >>> + qemu_del_vm_change_state_handler(bus->vmstate); >>> + } >>> + } >>> +} >>> >>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h >>> index 7824bc3..2103261 100644 >>> --- a/include/hw/ide/internal.h >>> +++ b/include/hw/ide/internal.h >>> @@ -480,6 +480,7 @@ struct IDEBus { >>> uint8_t retry_unit; >>> int64_t retry_sector_num; >>> uint32_t retry_nsector; >>> + VMChangeStateEntry *vmstate; >>> }; >>> >>> #define TYPE_IDE_DEVICE "ide-device" >>> -- >>> 2.6.2 >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter @ 2016-08-11 16:40 Ashijeet Acharya 2016-08-11 18:45 ` [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() Ashijeet Acharya 0 siblings, 1 reply; 8+ messages in thread From: Ashijeet Acharya @ 2016-08-11 16:40 UTC (permalink / raw) To: Paolo Bonzini; +Cc: John Snow, QEMU Developers On Thu, Aug 11, 2016 at 10:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> I think we should do >> >> s->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); >> instead of >> qemu_add_vm_change_state_handler(ide_restart_cb, bus); >> >> in ide_register_restart_cb() in hw/ide/core.c to store the returned >> pointer to memory to avoid a possible memory leak I guess and >> introduce a VMChangeStateEntry field in struct AHCIState to handle >> this. Same can then further be used with >> qemu_del_vm_change_state_handler() in ahci_unrealize() to free things >> up. > > Yes, this is correct. Thanks! > Alright I will make these changes and include them in my patch. Ashijeet > Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-08-11 16:40 [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter Ashijeet Acharya @ 2016-08-11 18:45 ` Ashijeet Acharya 2016-08-12 9:58 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Ashijeet Acharya @ 2016-08-11 18:45 UTC (permalink / raw) To: jsnow; +Cc: pbonzini, qemu-devel, Ashijeet Acharya Introduce VMChangeStateEntry parameter in ide_register_restart_cb() to handle possible memory leak from qemu_add_vm_change_state_handler(). Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- hw/ide/ahci.c | 2 +- hw/ide/cmd646.c | 2 +- hw/ide/core.c | 4 ++-- hw/ide/isa.c | 3 ++- hw/ide/piix.c | 2 +- hw/ide/via.c | 2 +- include/hw/ide/ahci.h | 1 + include/hw/ide/internal.h | 2 +- include/hw/ide/pci.h | 1 + 9 files changed, 11 insertions(+), 8 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index bcb9ff9..d7c96df 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1476,7 +1476,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) ad->port_no = i; ad->port.dma = &ad->dma; ad->port.dma->ops = &ahci_dma_ops; - ide_register_restart_cb(&ad->port); + ide_register_restart_cb(&ad->port, s->vmstate); } } diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 9ebb8d4..b906aa7 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -369,7 +369,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) bmdma_init(&d->bus[i], &d->bmdma[i], d); d->bmdma[i].bus = &d->bus[i]; - ide_register_restart_cb(&d->bus[i]); + ide_register_restart_cb(&d->bus[i], d->vmstate); } vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d); diff --git a/hw/ide/core.c b/hw/ide/core.c index d117b7c..0a27d4d 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2578,10 +2578,10 @@ static void ide_restart_cb(void *opaque, int running, RunState state) } } -void ide_register_restart_cb(IDEBus *bus) +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e) { if (bus->dma->ops->restart_dma) { - qemu_add_vm_change_state_handler(ide_restart_cb, bus); + e = qemu_add_vm_change_state_handler(ide_restart_cb, bus); } } diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 40213d6..74a5078 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -45,6 +45,7 @@ typedef struct ISAIDEState { uint32_t iobase2; uint32_t isairq; qemu_irq irq; + VMChangeStateEntry *vmstate; } ISAIDEState; static void isa_ide_reset(DeviceState *d) @@ -75,7 +76,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) isa_init_irq(isadev, &s->irq, s->isairq); ide_init2(&s->bus, s->irq); vmstate_register(dev, 0, &vmstate_ide_isa, s); - ide_register_restart_cb(&s->bus); + ide_register_restart_cb(&s->bus, s->vmstate); } ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq, diff --git a/hw/ide/piix.c b/hw/ide/piix.c index c190fca..2c67508 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -144,7 +144,7 @@ static void pci_piix_init_ports(PCIIDEState *d) { bmdma_init(&d->bus[i], &d->bmdma[i], d); d->bmdma[i].bus = &d->bus[i]; - ide_register_restart_cb(&d->bus[i]); + ide_register_restart_cb(&d->bus[i], d->vmstate); } } diff --git a/hw/ide/via.c b/hw/ide/via.c index 5b32ecb..82fbbf0 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -167,7 +167,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) { bmdma_init(&d->bus[i], &d->bmdma[i], d); d->bmdma[i].bus = &d->bus[i]; - ide_register_restart_cb(&d->bus[i]); + ide_register_restart_cb(&d->bus[i], d->vmstate); } } diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h index 0ca7c65..fa4a680 100644 --- a/include/hw/ide/ahci.h +++ b/include/hw/ide/ahci.h @@ -298,6 +298,7 @@ typedef struct AHCIState { int32_t ports; qemu_irq irq; AddressSpace *as; + VMChangeStateEntry *vmstate; } AHCIState; typedef struct AHCIPCIState { diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 7824bc3..a95e6e9 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -605,7 +605,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind, int chs_trans); void ide_init2(IDEBus *bus, qemu_irq irq); void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2); -void ide_register_restart_cb(IDEBus *bus); +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e); void ide_exec_cmd(IDEBus *bus, uint32_t val); diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index dbc6a03..95df1c0 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -57,6 +57,7 @@ typedef struct PCIIDEState { uint32_t secondary; /* used only for cmd646 */ MemoryRegion bmdma_bar; CMD646BAR cmd646_bar[2]; /* used only for cmd646 */ + VMChangeStateEntry *vmstate; } PCIIDEState; -- 2.6.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-08-11 18:45 ` [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() Ashijeet Acharya @ 2016-08-12 9:58 ` Paolo Bonzini 2016-08-16 13:55 ` Ashijeet Acharya 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-08-12 9:58 UTC (permalink / raw) To: Ashijeet Acharya, jsnow; +Cc: qemu-devel On 11/08/2016 20:45, Ashijeet Acharya wrote: > Introduce VMChangeStateEntry parameter in ide_register_restart_cb() to handle possible memory leak from qemu_add_vm_change_state_handler(). > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > hw/ide/ahci.c | 2 +- > hw/ide/cmd646.c | 2 +- > hw/ide/core.c | 4 ++-- > hw/ide/isa.c | 3 ++- > hw/ide/piix.c | 2 +- > hw/ide/via.c | 2 +- > include/hw/ide/ahci.h | 1 + > include/hw/ide/internal.h | 2 +- > include/hw/ide/pci.h | 1 + > 9 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index bcb9ff9..d7c96df 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1476,7 +1476,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) > ad->port_no = i; > ad->port.dma = &ad->dma; > ad->port.dma->ops = &ahci_dma_ops; > - ide_register_restart_cb(&ad->port); > + ide_register_restart_cb(&ad->port, s->vmstate); You need more than one vmstate handler here... > } > } > > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 9ebb8d4..b906aa7 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -369,7 +369,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus = &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); ... and here. It's probably simplest to add the field to IDEBus. You're also missing the calls to qemu_del_vm_change_state_handler. They should also be conditional on bus->dma->ops->restart_dma. I suggest creating an idebus_unrealize function to do so, and registering it in hw/ide/qdev.c with k->unrealize = idebus_unrealize; Thanks, Paolo > } > > vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d); > diff --git a/hw/ide/core.c b/hw/ide/core.c > index d117b7c..0a27d4d 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2578,10 +2578,10 @@ static void ide_restart_cb(void *opaque, int running, RunState state) > } > } > > -void ide_register_restart_cb(IDEBus *bus) > +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e) > { > if (bus->dma->ops->restart_dma) { > - qemu_add_vm_change_state_handler(ide_restart_cb, bus); > + e = qemu_add_vm_change_state_handler(ide_restart_cb, bus); > } > } > > diff --git a/hw/ide/isa.c b/hw/ide/isa.c > index 40213d6..74a5078 100644 > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -45,6 +45,7 @@ typedef struct ISAIDEState { > uint32_t iobase2; > uint32_t isairq; > qemu_irq irq; > + VMChangeStateEntry *vmstate; > } ISAIDEState; > > static void isa_ide_reset(DeviceState *d) > @@ -75,7 +76,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) > isa_init_irq(isadev, &s->irq, s->isairq); > ide_init2(&s->bus, s->irq); > vmstate_register(dev, 0, &vmstate_ide_isa, s); > - ide_register_restart_cb(&s->bus); > + ide_register_restart_cb(&s->bus, s->vmstate); > } > > ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq, > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index c190fca..2c67508 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -144,7 +144,7 @@ static void pci_piix_init_ports(PCIIDEState *d) { > > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus = &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); > } > } > > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 5b32ecb..82fbbf0 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -167,7 +167,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) { > > bmdma_init(&d->bus[i], &d->bmdma[i], d); > d->bmdma[i].bus = &d->bus[i]; > - ide_register_restart_cb(&d->bus[i]); > + ide_register_restart_cb(&d->bus[i], d->vmstate); > } > } > > diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h > index 0ca7c65..fa4a680 100644 > --- a/include/hw/ide/ahci.h > +++ b/include/hw/ide/ahci.h > @@ -298,6 +298,7 @@ typedef struct AHCIState { > int32_t ports; > qemu_irq irq; > AddressSpace *as; > + VMChangeStateEntry *vmstate; > } AHCIState; > > typedef struct AHCIPCIState { > diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h > index 7824bc3..a95e6e9 100644 > --- a/include/hw/ide/internal.h > +++ b/include/hw/ide/internal.h > @@ -605,7 +605,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind, > int chs_trans); > void ide_init2(IDEBus *bus, qemu_irq irq); > void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2); > -void ide_register_restart_cb(IDEBus *bus); > +void ide_register_restart_cb(IDEBus *bus, VMChangeStateEntry *e); > > void ide_exec_cmd(IDEBus *bus, uint32_t val); > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index dbc6a03..95df1c0 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -57,6 +57,7 @@ typedef struct PCIIDEState { > uint32_t secondary; /* used only for cmd646 */ > MemoryRegion bmdma_bar; > CMD646BAR cmd646_bar[2]; /* used only for cmd646 */ > + VMChangeStateEntry *vmstate; > } PCIIDEState; > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-08-12 9:58 ` Paolo Bonzini @ 2016-08-16 13:55 ` Ashijeet Acharya 2016-08-16 17:08 ` John Snow 0 siblings, 1 reply; 8+ messages in thread From: Ashijeet Acharya @ 2016-08-16 13:55 UTC (permalink / raw) To: jsnow; +Cc: pbonzini, qemu-devel, Ashijeet Acharya Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- hw/ide/core.c | 2 +- hw/ide/qdev.c | 14 ++++++++++++++ include/hw/ide/internal.h | 1 + 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 45b6df1..eecbb47 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) void ide_register_restart_cb(IDEBus *bus) { if (bus->dma->ops->restart_dma) { - qemu_add_vm_change_state_handler(ide_restart_cb, bus); + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); } } diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 67c76bf..6f75f77 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -31,6 +31,7 @@ /* --------------------------------- */ static char *idebus_get_fw_dev_path(DeviceState *dev); +static void idebus_unrealize(DeviceState *qdev, Error **errp); static Property ide_props[] = { DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) k->init = ide_qdev_init; set_bit(DEVICE_CATEGORY_STORAGE, k->categories); k->bus_type = TYPE_IDE_BUS; + k->unrealize = idebus_unrealize; k->props = ide_props; } @@ -368,3 +370,15 @@ static void ide_register_types(void) } type_init(ide_register_types) + +static void idebus_unrealize(DeviceState *qdev, Error **errp) +{ + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); + + if (bus->dma->ops->restart_dma) { + if (bus->vmstate) { + qemu_del_vm_change_state_handler(bus->vmstate); + } + } +} diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 7824bc3..2103261 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -480,6 +480,7 @@ struct IDEBus { uint8_t retry_unit; int64_t retry_sector_num; uint32_t retry_nsector; + VMChangeStateEntry *vmstate; }; #define TYPE_IDE_DEVICE "ide-device" -- 2.6.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() 2016-08-16 13:55 ` Ashijeet Acharya @ 2016-08-16 17:08 ` John Snow 0 siblings, 0 replies; 8+ messages in thread From: John Snow @ 2016-08-16 17:08 UTC (permalink / raw) To: Ashijeet Acharya; +Cc: pbonzini, qemu-devel Can you please resend this not as a reply to this discussion thread? http://wiki.qemu.org/Contribute/SubmitAPatch Thank you, --js On 08/16/2016 09:55 AM, Ashijeet Acharya wrote: > Fix a memory leak in ide_register_restart_cb() in hw/ide/core.c and add idebus_unrealize() in hw/ide/qdev.c to have calls to qemu_del_vm_change_state_handler() to deal with the dangling change state handler during hot-unplugging ide devices which might lead to a crash. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > hw/ide/core.c | 2 +- > hw/ide/qdev.c | 14 ++++++++++++++ > include/hw/ide/internal.h | 1 + > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 45b6df1..eecbb47 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2582,7 +2582,7 @@ static void ide_restart_cb(void *opaque, int running, RunState state) > void ide_register_restart_cb(IDEBus *bus) > { > if (bus->dma->ops->restart_dma) { > - qemu_add_vm_change_state_handler(ide_restart_cb, bus); > + bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus); > } > } > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index 67c76bf..6f75f77 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -31,6 +31,7 @@ > /* --------------------------------- */ > > static char *idebus_get_fw_dev_path(DeviceState *dev); > +static void idebus_unrealize(DeviceState *qdev, Error **errp); > > static Property ide_props[] = { > DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1), > @@ -345,6 +346,7 @@ static void ide_device_class_init(ObjectClass *klass, void *data) > k->init = ide_qdev_init; > set_bit(DEVICE_CATEGORY_STORAGE, k->categories); > k->bus_type = TYPE_IDE_BUS; > + k->unrealize = idebus_unrealize; > k->props = ide_props; > } > > @@ -368,3 +370,15 @@ static void ide_register_types(void) > } > > type_init(ide_register_types) > + > +static void idebus_unrealize(DeviceState *qdev, Error **errp) > +{ > + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); > + > + if (bus->dma->ops->restart_dma) { > + if (bus->vmstate) { > + qemu_del_vm_change_state_handler(bus->vmstate); > + } > + } > +} > > diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h > index 7824bc3..2103261 100644 > --- a/include/hw/ide/internal.h > +++ b/include/hw/ide/internal.h > @@ -480,6 +480,7 @@ struct IDEBus { > uint8_t retry_unit; > int64_t retry_sector_num; > uint32_t retry_nsector; > + VMChangeStateEntry *vmstate; > }; > > #define TYPE_IDE_DEVICE "ide-device" > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-01 15:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-16 17:10 [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() Ashijeet Acharya 2016-09-01 5:31 ` Ashijeet Acharya 2016-09-01 15:43 ` Paolo Bonzini 2016-09-01 15:45 ` Ashijeet Acharya -- strict thread matches above, loose matches on Subject: below -- 2016-08-11 16:40 [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter Ashijeet Acharya 2016-08-11 18:45 ` [Qemu-devel] [PATCH] Fix memory leak in ide_register_restart_cb() Ashijeet Acharya 2016-08-12 9:58 ` Paolo Bonzini 2016-08-16 13:55 ` Ashijeet Acharya 2016-08-16 17:08 ` John Snow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).