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 <david@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);
+    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,
-- 
2.9.4

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 <david@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.

> +    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,
> -- 
> 2.9.4
> 

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 <david@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).

> 
> > +    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

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 <david@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