Il 09/12/2013 16:08, Igor Mammedov ha scritto:
> On Mon, 09 Dec 2013 15:36:35 +0100
> Paolo Bonzini <pbonz...@redhat.com> 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 agree that's the right thing to do.
>> 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.
We could have separate check/plug methods. Only check can fail, it must
be idempotent, and it can be invoked while the device is still unrealized.
The reason I liked the interface, is because it removes the need for
each bus to add its own plug/unplug handling.
Paolo
>>> 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
>>
>
