Re: [Qemu-devel] [PATCH 02/11] pseries: Cleanup error handling of spapr_cpu_init()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

On Fri, Dec 11, 2015 at 02:15:38PM +0530, Bharata B Rao wrote:
> On Fri, Dec 11, 2015 at 5:41 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
> > That works for now, since it's only called from initial setup where an
> > error here means we really can't proceed.
> >
> > However, we'll want to handle this more flexibly for cpu hotplug in future
> > so generalize this using the error reporting infrastructure.  While we're
> > at it make a small cleanup in a related part of ppc_spapr_init() to use
> > the error infrastructure instead of an old-style explicit fprintf / exit.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 1a5500f..91396cc 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1615,7 +1615,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
> >      machine->boot_order = g_strdup(boot_device);
> >  }
> >
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> > +static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > +                           Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> >
> > @@ -1633,7 +1634,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
> >      }
> >
> >      if (cpu->max_compat) {
> > -        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
> > +        ppc_set_compat(cpu, cpu->max_compat, errp);
> >      }
> >
> >      xics_cpu_setup(spapr->icp, cpu);
> > @@ -1802,10 +1803,9 @@ static void ppc_spapr_init(MachineState *machine)
> >      for (i = 0; i < smp_cpus; i++) {
> >          cpu = cpu_ppc_init(machine->cpu_model);
> >          if (cpu == NULL) {
> > -            fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > -            exit(1);
> > +            error_setg(&error_fatal, "Unable to find PowerPC CPU definition");
> >          }
> > -        spapr_cpu_init(spapr, cpu);
> > +        spapr_cpu_init(spapr, cpu, &error_fatal);
> 
> Using error_fatal is fine here for now, but just want to sound out
> that with CPU hotplug, spapr_cpu_init() would move to ->plug() where
> we can't be calling this unconditionally with error_fatal as we need a
> graceful recovery for hotplugged CPUs and termination for boot CPUs.

Right... part of the point of the patch is that the &error_fatal is
moved out of spapr_cpu_init() into the caller.  Hotplug will add a new
caller which can use a different error handler.

> Otherwise, Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Regards,
> Bharata.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]