Quoting David Gibson (2017-06-06 20:33:07) > On Tue, Jun 06, 2017 at 04:37:27PM -0500, Michael Roth wrote: > > Quoting David Gibson (2017-06-06 08:05:32) > > > This function is fairly short, and so is its only caller. There's no > > > particular logical distinction between them, so fold them together. > > > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > --- > > > hw/ppc/spapr_pci.c | 53 > > > ++++++++++++++++++++++------------------------------- > > > 1 file changed, 22 insertions(+), 31 deletions(-) > > > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > > index 46e736d..a216f61 100644 > > > --- a/hw/ppc/spapr_pci.c > > > +++ b/hw/ppc/spapr_pci.c > > > @@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState > > > *phb, PCIDevice *dev, > > > return offset; > > > } > > > > > > -static void spapr_phb_add_pci_device(sPAPRDRConnector *drc, > > > - sPAPRPHBState *phb, > > > - PCIDevice *pdev, > > > - Error **errp) > > > -{ > > > - DeviceState *dev = DEVICE(pdev); > > > - void *fdt = NULL; > > > - int fdt_start_offset = 0, fdt_size; > > > - > > > - fdt = create_device_tree(&fdt_size); > > > - fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0); > > > - if (!fdt_start_offset) { > > > - error_setg(errp, "Failed to create pci child device tree node"); > > > - goto out; > > > - } > > > - > > > - spapr_drc_attach(drc, DEVICE(pdev), > > > - fdt, fdt_start_offset, !dev->hotplugged, errp); > > > -out: > > > - if (*errp) { > > > - g_free(fdt); > > > - } > > > -} > > > - > > > /* Callback to be called during DRC release. */ > > > void spapr_phb_remove_pci_device_cb(DeviceState *dev) > > > { > > > @@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler > > > *plug_handler, > > > Error *local_err = NULL; > > > PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))); > > > uint32_t slotnr = PCI_SLOT(pdev->devfn); > > > + void *fdt = NULL; > > > + int fdt_start_offset, fdt_size; > > > > > > /* if DR is disabled we don't need to do anything in the case of > > > * hotplug or coldplug callbacks > > > @@ -1438,10 +1416,10 @@ static void > > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler, > > > * we need to let them know it's not enabled > > > */ > > > if (plugged_dev->hotplugged) { > > > - error_setg(errp, QERR_BUS_NO_HOTPLUG, > > > + error_setg(&local_err, QERR_BUS_NO_HOTPLUG, > > > object_get_typename(OBJECT(phb))); > > > } > > > - return; > > > + goto out; > > > } > > > > > > g_assert(drc); > > > @@ -1452,16 +1430,23 @@ static void > > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler, > > > */ > > > if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] && > > > PCI_FUNC(pdev->devfn) != 0) { > > > - error_setg(errp, "PCI: slot %d function 0 already ocuppied by > > > %s," > > > + error_setg(&local_err, "PCI: slot %d function 0 already ocuppied > > > by %s," > > > " additional functions can no longer be exposed to > > > guest.", > > > slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name); > > > - return; > > > + goto out; > > > } > > > > > > - spapr_phb_add_pci_device(drc, phb, pdev, &local_err); > > > > Since we never used local_err outside propagating it and immediately > > bailing, and since we bail on errp in all prior callers, maybe we > > should just drop local_err completely in favor errp. > > That doesn't quite work. The reason for the local_err pattern is so > that we can tell locally if the error was triggered (errp might be > NULL, so checking *errp isn't safe).
Ah, of course, not sure how I keep forgetting that detail. > > > > > > + fdt = create_device_tree(&fdt_size); > > > + fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0); > > > + if (!fdt_start_offset) { > > > + error_setg(&local_err, "Failed to create pci child device tree > > > node"); > > > + goto out; > > > + } > > > + > > > + spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset, > > > + !plugged_dev->hotplugged, &local_err); > > > if (local_err) { > > > - error_propagate(errp, local_err); > > > - return; > > > + goto out; > > > } > > > > > > /* If this is function 0, signal hotplug for all the device > > > functions. > > > @@ -1485,6 +1470,12 @@ static void > > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler, > > > } > > > } > > > } > > > + > > > +out: > > > + if (local_err) { > > > + error_propagate(errp, local_err); > > > + g_free(fdt); > > > + } > > > } > > > > > > static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler, > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson