From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bovOs-0000zu-Db for qemu-devel@nongnu.org; Tue, 27 Sep 2016 12:38:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bovOp-0004H4-66 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 12:38:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bovOo-0004FT-Sa for qemu-devel@nongnu.org; Tue, 27 Sep 2016 12:38:31 -0400 References: <1474480412-18353-1-git-send-email-ashijeetacharya@gmail.com> From: John Snow Message-ID: <92cfa23c-43d7-3212-42f0-5f9ca85c163e@redhat.com> Date: Tue, 27 Sep 2016 12:38:26 -0400 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] ide: Fix memory leak in ide_register_restart_cb() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashijeet Acharya , Paolo Bonzini Cc: QEMU Developers On 09/22/2016 01:47 AM, Ashijeet Acharya wrote: > On Thu, Sep 22, 2016 at 1:08 AM, John Snow wrote: >> >> >> On 09/21/2016 01: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. >>> >> >> Minor rebase issue, but it's trivially resolved. >> >> >>> Signed-off-by: Ashijeet Acharya >>> --- >>> Changes in v2: >>> -v1 was corrupted at line 64 >>> -Move idebus_unrealize() below ide_bus_class_init() >>> --- >>> hw/ide/core.c | 2 +- >>> hw/ide/qdev.c | 13 +++++++++++++ >>> include/hw/ide/internal.h | 1 + >>> 3 files changed, 15 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..c94f9f8 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,17 @@ 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->dma->ops->restart_dma) { >>> + if (bus->vmstate) { >>> + qemu_del_vm_change_state_handler(bus->vmstate); >>> + } >>> + } >>> +} >>> + >> >> >> Naive question: I saw Paolo say that this should be conditional on >> bus->dma->ops->restart_dma -- Why can't we just say if (bus->vmstate) ? >> >> I see that we only allocate the change state handler when restart_dma is >> present, but this makes this portion of the code look funny when if >> (bus->vmstate) would be just as simple, no? >> > > I had a similar thought, because other pieces of code also do it in > "if (bus->vmstate)" manner but maybe I was missing on something > important and ended up coding how Paolo instructed me. > I think we can use the smaller conditional -- less prone to error that way in case we change the conditional on how it was constructed. Let's do that and I'll merge this for you finally. >> (Unless we can't rely on its NULL initialization or some such.) > > Maybe, but shouldn't the same logic apply elsewhere then? > >> >>> static const TypeInfo ide_bus_info = { >>> .name = TYPE_IDE_BUS, >>> .parent = TYPE_BUS, >>> @@ -355,6 +367,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; >> >> >> Hmm, I was going to complain and say that 'vmstate' is a generic name for >> this field, but it's by far the most common name for this task. I cede! >> >>> }; >>> >>> #define TYPE_IDE_DEVICE "ide-device" >>> >> >> Seems good otherwise, thank you! > Thanks! > > Ashijeet >