On Tue, Sep 27, 2016 at 10:08 PM, John Snow <js...@redhat.com> wrote:
>
>
> On 09/22/2016 01:47 AM, Ashijeet Acharya wrote:
>>
>> On Thu, Sep 22, 2016 at 1:08 AM, John Snow <js...@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 <ashijeetacha...@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.
Sure, I will send v3 ASAP.
Ashijeet
>
>
>>> (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
>>
>
