| Message ID | 1471367421-10431-1-git-send-email-ashijeetacharya@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >>
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 >>>
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"
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(-)