Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler

[David Hildenbrand]

On 03/10/2018 08:29, David Gibson wrote:
> On Wed, Sep 26, 2018 at 11:42:13AM +0200, David Hildenbrand wrote:
>> The unplug and unplug_request handlers are special: They are not
>> executed when unrealizing a device, but rather trigger the removal of a
>> device from device_del() via object_unparent() - to effectively
>> unrealize a device.
>>
>> If such a device has a child bus and another device attached to
>> that bus (e.g. how virtio devices are created with their proxy device),
>> we will not get a call to the unplug handler. As we want to support
>> hotplug handlers (and especially also some unplug logic to undo resource
>> assignment) for such devices, we cannot simply call the unplug handler
>> when unrealizing - it has a different semantic ("trigger removal").
>>
>> To handle this scenario, we need a do_unplug handler, that will be
>> executed for all devices with a hotplug handler.
>>
>> While at it, introduce hotplug_fn_nofail and fix a spelling mistake in
>> a comment.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/core/hotplug.c    | 10 ++++++++++
>>  hw/core/qdev.c       |  6 ++++++
>>  include/hw/hotplug.h | 26 ++++++++++++++++++++++++--
>>  3 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>> index 2253072d0e..e7a68d5160 100644
>> --- a/hw/core/hotplug.c
>> +++ b/hw/core/hotplug.c
>> @@ -45,6 +45,16 @@ void hotplug_handler_post_plug(HotplugHandler 
>> *plug_handler,
>>      }
>>  }
>>  
>> +void hotplug_handler_do_unplug(HotplugHandler *plug_handler,
>> +                                 DeviceState *plugged_dev)
> 
> Hrm.  I really dislike things named "do_X".  The "do" rarely adds any
> useful meaning.  And when there's also something called just plain
> "X", it's *always* unclear how they relate to each other.
> 
> That's doubly true when it's a general interface like this, rather
> than just some local functions.
> 

Yes, the naming is not the best, but I didn't want to rename all unplug
handlers before we have an agreement on how to proceed. My concept would
be that

1. unplug() is renamed to trigger_unplug(). unplug_request() remains.
2. do_unplug() is renamed to pre_unplug() (just like pre_plug())
3. we might have in addition unplug() after realize(false) - just like
plug()

trigger_unplug() would perform checks and result in object_unparent().
>From there, all cleanup/unplugging would be performed via the unrealize
chain, just like we have for realize() now. That would allow to properly
unplug complete device hierarchies.

But Igor rather wants one hotplug handler chain, and no dedicated
hotplug handlers for other devices in the device hierarchy. As long as
there are no other opinions I guess I will have to go into the direction
Igor proposes to get virtio-pmem and friends upstream.

-- 

Thanks,

David / dhildenb