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

[John Snow <jsnow@redhat.com>]

On 09/22/2016 01:47 AM, Ashijeet Acharya wrote:
> On Thu, Sep 22, 2016 at 1:08 AM, John Snow <jsnow@redhat.com> 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 <ashijeetacharya@gmail.com>
>>> ---
>>> 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
>