Re: qom device lifecycle interaction with hotplug/hotunplug ?

[Jens Freimann <jfreimann@redhat.com>]

On Wed, Dec 11, 2019 at 01:52:33PM +0100, Damien Hedde wrote:
>On 12/4/19 7:51 PM, Eduardo Habkost wrote:
>> On Wed, Dec 04, 2019 at 05:21:25PM +0100, Jens Freimann wrote:
>>> On Wed, Dec 04, 2019 at 11:35:37AM -0300, Eduardo Habkost wrote:
>>>> On Wed, Dec 04, 2019 at 10:18:24AM +0100, Jens Freimann wrote:
>>>>> On Tue, Dec 03, 2019 at 06:40:04PM -0300, Eduardo Habkost wrote:
>>>>>> +jfreimann, +mst
>And if migration fails this same device is plugged back using
>failover_replug_primary():
>> static bool failover_replug_primary(VirtIONet *n, Error **errp)
>> {
>>     [...]
>>     qdev_set_parent_bus(n->primary_dev, n->primary_bus);
>>     [...]
>>     hotplug_ctrl = qdev_get_hotplug_handler(n->primary_dev);
>>     if (hotplug_ctrl) {
>>         hotplug_handler_pre_plug(hotplug_ctrl, n->primary_dev, &err);
>>         if (err) {
>>             goto out;
>>         }
>>         hotplug_handler_plug(hotplug_ctrl, n->primary_dev, errp);
>>     }
>>     [...]
>> }
>
>My concern is about the qdev_set_parent_bus() call above (because I
>touch this function in my series) and don't want to have side effects there.
>
>I looked at the code and thought the partial unplug has the effect of
>cutting off the unplug procedure just before doing the qdev real unplug.
>In pcie_unplug_device() we return before doing the object_unparent():
>> static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, ...
>>  {
>>      [...]
>>      if (dev->partially_hotplugged) {
>>          dev->qdev.pending_deleted_event = false;
>>          return;
>>      }
>>      hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort);
>>      object_unparent(OBJECT(dev));
>>  }
>
>From my understanding, object_unparent() is the only way to really
>unplug a device from a bus regarding qdev (and it also unrealizes the
>device). So I have the feeling that qdev_set_parent_bus() here is a
>no-op (because primary_dev is already on primary_bus).

I tested this now and it really is a no-op. It was required in a
earlier version of the patches and I missed to remove it when I
reworked the re-plug functionality.

I will send a patch to remove it.

regards
Jens