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

[Eric Blake <eblake@redhat.com>]

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

On 12/10/2015 05:11 PM, David Gibson 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(-)

> @@ -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);

Pre-patch: you can't reach the xics_cpu_setup() call on error.

Post-patch: depending on what the caller passed in, you can fall through
to xics_cpu_setup() with a potentially incomplete cpu.

I think a more robust solution is probably along the lines of:

Error *err = NULL;
if (cpu->max_compat) {
    ppc_set_compat(cpu, cpu->max_compat, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }
}
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);

Of course, in _this_ patch, since the (only) caller is passing
&error_fatal, you don't hit the situation of fallthrough to
xics_cpu_setup().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]