Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.

[Igor Mammedov]

On Mon, 09 Dec 2013 15:36:35 +0100
Paolo Bonzini <address@hidden> wrote:

> Il 09/12/2013 15:14, Igor Mammedov ha scritto:
> >> > 
> >> > Makes sense.  realize() for the "plug" handler, and qdev_unplug for the
> >> > unplug handler, I guess.
> > Just to be sure, I've meant not DEVICE.realize() but each device specific
> > one.
> 
> If it's each specific device, then why should the hotplug handler link
> be in DeviceState?
The reason I've put it there is to eventually replace allow_hotplug field with 
it,
it also reduces code duplication (i.e. we wont' have to add it in PCIDevice,
DimmDevice ...) and potentially allows to use NULL for error in
property lookup since each bus will have it.

> 
> I think it should be in device_set_realized.
if we dub it nofail then it's fine, otherwise failure path becomes more 
complicated.

Calling handler in specific device realize() allows to gracefully abort
realize() since that device knows what needs to be done to do so:
For example:
 @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
 ...
+            hdc->hotplug(hotplug_dev, qdev, &local_err);
+            if (error_is_set(&local_err)) {
+                int r = pci_unregister_device(&pci_dev->qdev);

Calling handler in realize will not allow to do it.
It's just much more flexible to call handler from specific device since it knows
when it's the best to call handler and how to deal with failure.

> 
> > qdev_unplug() might work for now, but I haven't checked all devices that
> > might use interface and if it would break anything. Ideally it should be
> > in device's unrealize() complementing realize() part.
> > 
> > I'd wait till all buses converted to new interface before attempting to
> > generalize current plug/unplug call pathes though.
> 
> I agree that adding a default behavior for no link probably requires
> conversion of all buses.  However, looking for the link in the generic
> code can be done now.
> 
> Paolo
>