| Message ID | 1474995212-10580-1-git-send-email-ashijeetacharya@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/27/2016 12:53 PM, 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. > In the future, please rebase your patches on top of the current git master when you resend. > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > Changes in v3: > -Use smaller conditional (bus->vmstate) only > --- > hw/ide/core.c | 2 +- > hw/ide/qdev.c | 11 +++++++++++ > include/hw/ide/internal.h | 1 + > 3 files changed, 13 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 2eb055a..7e186bd 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), > @@ -44,6 +45,15 @@ static void ide_bus_class_init(ObjectClass *klass, void *data) > k->get_fw_dev_path = idebus_get_fw_dev_path; > } > > +static void idebus_unrealize(DeviceState *qdev, Error **errp) > +{ > + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); > + > + if (bus->vmstate) { > + qemu_del_vm_change_state_handler(bus->vmstate); Also in the future, keep an eye out for whitespace changes, too. > + } > +} > + > static const TypeInfo ide_bus_info = { > .name = TYPE_IDE_BUS, > .parent = TYPE_BUS, > @@ -355,6 +365,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; > } > > 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" > I made the minor rebase and whitespace edit in my tree, and with that: Reviewed-by: John Snow <jsnow@redhat.com> Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js
On Wed, Sep 28, 2016 at 3:58 AM, John Snow <jsnow@redhat.com> wrote: > > > On 09/27/2016 12:53 PM, 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. >> > > In the future, please rebase your patches on top of the current git master > when you resend. Yeah, sorry about that. > > >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> Changes in v3: >> -Use smaller conditional (bus->vmstate) only >> --- >> hw/ide/core.c | 2 +- >> hw/ide/qdev.c | 11 +++++++++++ >> include/hw/ide/internal.h | 1 + >> 3 files changed, 13 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 2eb055a..7e186bd 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), >> @@ -44,6 +45,15 @@ static void ide_bus_class_init(ObjectClass *klass, void >> *data) >> k->get_fw_dev_path = idebus_get_fw_dev_path; >> } >> >> +static void idebus_unrealize(DeviceState *qdev, Error **errp) >> +{ >> + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); >> + >> + if (bus->vmstate) { >> + qemu_del_vm_change_state_handler(bus->vmstate); > > > Also in the future, keep an eye out for whitespace changes, too. > >> + } >> +} >> + >> static const TypeInfo ide_bus_info = { >> .name = TYPE_IDE_BUS, >> .parent = TYPE_BUS, >> @@ -355,6 +365,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; >> } >> >> 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" >> > > I made the minor rebase and whitespace edit in my tree, and with that: > > Reviewed-by: John Snow <jsnow@redhat.com> > > Thanks, applied to my IDE tree: Great! Thanks a lot. Ashijeet > > https://github.com/jnsnow/qemu/commits/ide > https://github.com/jnsnow/qemu.git > > --js
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 2eb055a..7e186bd 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), @@ -44,6 +45,15 @@ static void ide_bus_class_init(ObjectClass *klass, void *data) k->get_fw_dev_path = idebus_get_fw_dev_path; } +static void idebus_unrealize(DeviceState *qdev, Error **errp) +{ + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); + + if (bus->vmstate) { + qemu_del_vm_change_state_handler(bus->vmstate); + } +} + static const TypeInfo ide_bus_info = { .name = TYPE_IDE_BUS, .parent = TYPE_BUS, @@ -355,6 +365,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; } 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> --- Changes in v3: -Use smaller conditional (bus->vmstate) only --- hw/ide/core.c | 2 +- hw/ide/qdev.c | 11 +++++++++++ include/hw/ide/internal.h | 1 + 3 files changed, 13 insertions(+), 1 deletion(-)