Re: [Qemu-devel] [PATCH v2] ide: Fix memory leak in ide_register_restart_cb()

[John Snow]

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 <address@hidden>
> ---
> 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!