[06/11] pseries: Improve error handling in find_unknown_sysbus_device()

Use error_setg() to return an error instead of using an explicit exit().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

On 11/12/15 01:11, David Gibson wrote:
> Use error_setg() to return an error instead of using an explicit exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0ff09b9..fd16db4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
>  
>  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {
> +    Error **errp = opaque;
>      bool matched = false;
>  
>      if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>      }
>  
>      if (!matched) {
> -        error_report("Device %s is not supported by this machine yet.",
> -                     qdev_fw_name(DEVICE(sbdev)));
> -        exit(1);
> +        error_setg(errp,
> +                   "Device %s is not supported by this machine yet",
> +                   qdev_fw_name(DEVICE(sbdev)));
>      }
>  
>      return 0;
> @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
>      uint32_t rtas_limit;
>  
>      /* Check for unknown sysbus devices */
> -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
>  
>      /* Reset the hash table & recalc the RMA */
>      spapr_reset_htab(spapr, &error_abort);

Can this error be triggered by the user (by plugging certain devices or
so)? If so, I'm not sure whether error_abort is the right thing to use
here ... it's always ugly if the user can trigger an abort directly.
Maybe error_fatal would be better instead?

 Thomas

On 12/10/2015 05:11 PM, David Gibson wrote:
> Use error_setg() to return an error instead of using an explicit exit().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

> +++ b/hw/ppc/spapr.c
> @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
>  
>  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
>  {

>      if (!matched) {
> -        error_report("Device %s is not supported by this machine yet.",
> -                     qdev_fw_name(DEVICE(sbdev)));
> -        exit(1);
> +        error_setg(errp,
> +                   "Device %s is not supported by this machine yet",
> +                   qdev_fw_name(DEVICE(sbdev)));
>      }
>  
>      return 0;

It looks like find_unknown_sysbus_device is designed to be called in a
loop, and that returning 0 lets the loop continue.

> @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
>      uint32_t rtas_limit;
>  
>      /* Check for unknown sysbus devices */
> -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);

If a caller passes something other than &error_abort as the opaque, AND
the error condition occurs more than once in the loop iteration, then
you really need to change find_unknown_sysbus_device() to return
non-zero after raising error on the first failure, so that the loop
doesn't continue on to a second pass and attempt an error_setg() onto an
already-set error.

Of course, since _this_ patch uses &error_abort as the only client, the
loop will never continue after the first failure, but it's more robust
to be clean without having to audit the callers.

On Fri, Dec 11, 2015 at 10:49:40AM +0100, Thomas Huth wrote:
> On 11/12/15 01:11, David Gibson wrote:
> > Use error_setg() to return an error instead of using an explicit exit().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0ff09b9..fd16db4 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
> >  
> >  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >  {
> > +    Error **errp = opaque;
> >      bool matched = false;
> >  
> >      if (object_dynamic_cast(OBJECT(sbdev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > @@ -1114,9 +1115,9 @@ static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >      }
> >  
> >      if (!matched) {
> > -        error_report("Device %s is not supported by this machine yet.",
> > -                     qdev_fw_name(DEVICE(sbdev)));
> > -        exit(1);
> > +        error_setg(errp,
> > +                   "Device %s is not supported by this machine yet",
> > +                   qdev_fw_name(DEVICE(sbdev)));
> >      }
> >  
> >      return 0;
> > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
> >      uint32_t rtas_limit;
> >  
> >      /* Check for unknown sysbus devices */
> > -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> > +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
> >  
> >      /* Reset the hash table & recalc the RMA */
> >      spapr_reset_htab(spapr, &error_abort);
> 
> Can this error be triggered by the user (by plugging certain devices or
> so)? If so, I'm not sure whether error_abort is the right thing to use
> here ... it's always ugly if the user can trigger an abort directly.
> Maybe error_fatal would be better instead?

Hm.  So, yes, I think it can be triggered by the user.  I used
&error_abort on the grounds that this is during reset - after the
initial setup phase.

Markus, do you have an opinion on what the right handling here is?

On Fri, Dec 11, 2015 at 08:15:39AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 PM, David Gibson wrote:
> > Use error_setg() to return an error instead of using an explicit exit().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> 
> > +++ b/hw/ppc/spapr.c
> > @@ -1107,6 +1107,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
> >  
> >  static int find_unknown_sysbus_device(SysBusDevice *sbdev, void *opaque)
> >  {
> 
> >      if (!matched) {
> > -        error_report("Device %s is not supported by this machine yet.",
> > -                     qdev_fw_name(DEVICE(sbdev)));
> > -        exit(1);
> > +        error_setg(errp,
> > +                   "Device %s is not supported by this machine yet",
> > +                   qdev_fw_name(DEVICE(sbdev)));
> >      }
> >  
> >      return 0;
> 
> It looks like find_unknown_sysbus_device is designed to be called in a
> loop, and that returning 0 lets the loop continue.
> 
> > @@ -1151,7 +1152,7 @@ static void ppc_spapr_reset(void)
> >      uint32_t rtas_limit;
> >  
> >      /* Check for unknown sysbus devices */
> > -    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
> > +    foreach_dynamic_sysbus_device(find_unknown_sysbus_device, &error_abort);
> 
> If a caller passes something other than &error_abort as the opaque, AND
> the error condition occurs more than once in the loop iteration, then
> you really need to change find_unknown_sysbus_device() to return
> non-zero after raising error on the first failure, so that the loop
> doesn't continue on to a second pass and attempt an error_setg() onto an
> already-set error.

Ah, good catch.  I'll correct in the next spin.

> Of course, since _this_ patch uses &error_abort as the only client, the
> loop will never continue after the first failure, but it's more robust
> to be clean without having to audit the callers.
>