From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmnLY-00068u-J4 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 15:38:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmnLU-0004ur-AV for qemu-devel@nongnu.org; Wed, 21 Sep 2016 15:38:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60164) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmnLU-0004uK-0N for qemu-devel@nongnu.org; Wed, 21 Sep 2016 15:38:16 -0400 References: <1474480412-18353-1-git-send-email-ashijeetacharya@gmail.com> From: John Snow Message-ID: Date: Wed, 21 Sep 2016 15:38:14 -0400 MIME-Version: 1.0 In-Reply-To: <1474480412-18353-1-git-send-email-ashijeetacharya@gmail.com> 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 Cc: qemu-devel@nongnu.org, Paolo Bonzini 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? (Unless we can't rely on its NULL initialization or some such.) > 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!