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.
