On 20.11.18 15:13, Igor Mammedov wrote:
> On Tue, 20 Nov 2018 11:11:46 +0100
> David Hildenbrand <da...@redhat.com> wrote:
>
>>>> diff --git a/hw/pci-bridge/pcie_pci_bridge.c
>>>> b/hw/pci-bridge/pcie_pci_bridge.c
>>>> index c634353b06..7c667bc97c 100644
>>>> --- a/hw/pci-bridge/pcie_pci_bridge.c
>>>> +++ b/hw/pci-bridge/pcie_pci_bridge.c
>>>> @@ -150,6 +150,19 @@ static void pcie_pci_bridge_plug_cb(HotplugHandler
>>>> *hotplug_dev,
>>>> shpc_device_plug_cb(hotplug_dev, dev, errp);
>>>> }
>>>>
>>>> +static void pcie_pci_bridge_unplug_cb(HotplugHandler *hotplug_dev,
>>>> + DeviceState *dev, Error **errp)
>>>> +{
>>>> + PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
>>>> +
>>>> + if (!shpc_present(pci_hotplug_dev)) {
>>>> + error_setg(errp, "standard hotplug controller has been disabled
>>>> for "
>>>> + "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
>>> Is it possible to reach here at all?
>>
>> Right, this should right now not be possible. I'll turn this into an
>>
>> g_assert(shpc_present(pci_hotplug_dev));
>>
>> (if we every support surprise removal, we'll have to rethink either way)
>>
>>>
>>>> + return;
>>>> + }
>>>> + shpc_device_unplug_cb(hotplug_dev, dev, errp);
>>>> +}
>>>
>>> you probably can share this function impl. with pci_bridge_dev_unplug_cb()
>>> if you use object_get_typename().
>>
>> The same holds for all three functions (+ later pre_plug), however can
>> we be sure there won't be differences in the near future?
>>
>> If we don't expect these functions to differ, I can add a patch to
>> factor the existing two functions (plug/unplug) out.
> I'm not sure if they will differ or not in future,
> but right now it looks as premature splitting.
> It might be better to have a single function now and split it later
> when it is necessary.
>
Indeed, I already went ahead and sent a new version including the
suggested re-factoring.
Thanks!
--
Thanks,
David / dhildenb
