Re: [Qemu-devel] [PATCH 01/11] ppc: Cleanup error handling in ppc_set_compat()

[Thomas Huth <thuth@redhat.com>]

On 11/12/15 01:11, David Gibson wrote:
> Current ppc_set_compat() returns -1 for errors, and also (unconditionally)
> reports an error message.  The caller in h_client_architecture_support()
> may then report it again using an outdated fprintf().
> 
> Clean this up by using the modern error reporting mechanisms.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c              |  4 +---
>  hw/ppc/spapr_hcall.c        | 10 +++++-----
>  target-ppc/cpu.h            |  2 +-
>  target-ppc/translate_init.c | 13 +++++++------
>  4 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2d57ab0..1a5500f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1633,9 +1633,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
>      }
>  
>      if (cpu->max_compat) {
> -        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
> -            exit(1);
> -        }
> +        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
>      }
>  
>      xics_cpu_setup(spapr->icp, cpu);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index cebceea..8b0fcb3 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -837,7 +837,7 @@ static target_ulong cas_get_option_vector(int vector, target_ulong table)
>  typedef struct {
>      PowerPCCPU *cpu;
>      uint32_t cpu_version;
> -    int ret;
> +    Error *err;
>  } SetCompatState;
>  
>  static void do_set_compat(void *arg)
> @@ -845,7 +845,7 @@ static void do_set_compat(void *arg)
>      SetCompatState *s = arg;
>  
>      cpu_synchronize_state(CPU(s->cpu));
> -    s->ret = ppc_set_compat(s->cpu, s->cpu_version);
> +    ppc_set_compat(s->cpu, s->cpu_version, &s->err);
>  }
>  
>  #define get_compat_level(cpuver) ( \
> @@ -929,13 +929,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>              SetCompatState s = {
>                  .cpu = POWERPC_CPU(cs),
>                  .cpu_version = cpu_version,
> -                .ret = 0
> +                .err = NULL,
>              };
>  
>              run_on_cpu(cs, do_set_compat, &s);
>  
> -            if (s.ret < 0) {
> -                fprintf(stderr, "Unable to set compatibility mode\n");
> +            if (s.err) {
> +                error_report_err(s.err);
>                  return H_HARDWARE;
>              }
>          }
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 9706000..b3b89e6 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1210,7 +1210,7 @@ void ppc_store_msr (CPUPPCState *env, target_ulong value);
>  
>  void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
>  int ppc_get_compat_smt_threads(PowerPCCPU *cpu);
> -int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp);
>  
>  /* Time-base and decrementer management */
>  #ifndef NO_CPU_IO_DEFS
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index e88dc7f..83e5332 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9186,7 +9186,7 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
>      return ret;
>  }
>  
> -int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp)
>  {
>      int ret = 0;
>      CPUPPCState *env = &cpu->env;
> @@ -9208,12 +9208,13 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
>          break;
>      }
>  
> -    if (kvm_enabled() && kvmppc_set_compat(cpu, cpu->cpu_version) < 0) {
> -        error_report("Unable to set compatibility mode in KVM");
> -        ret = -1;
> +    if (kvm_enabled()) {
> +        ret = kvmppc_set_compat(cpu, cpu->cpu_version);
> +        if (ret < 0) {
> +            error_setg(errp, "Unable to set CPU compatibility mode in KVM: %s",
> +                       strerror(-ret));
> +        }
>      }
> -
> -    return ret;
>  }
>  
>  static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
> 

Looks good.

Reviewed-by: Thomas Huth <thuth@redhat.com>