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

2015-12-13 Thread David Gibson 
On Fri, Dec 11, 2015 at 06:58:12AM -0700, Eric Blake wrote:
> On 12/10/2015 05:11 PM, 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 
> > ---
> 
> > @@ -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));
> > +}
> 
> Could use error_setg_errno() here instead of manually calling strerror().

Ah, thanks, I hadn't spotted that function before.

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


signature.asc
Description: PGP signature

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

2015-12-11 Thread Eric Blake 
On 12/10/2015 05:11 PM, 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 
> ---

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

Could use error_setg_errno() here instead of manually calling strerror().

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



signature.asc
Description: OpenPGP digital signature

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

2015-12-11 Thread Thomas Huth 
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 
> ---
>  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, _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, >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, );
>  
> -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 = >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

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

2015-12-10 Thread David Gibson 
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 
---
 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, _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, >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, );
 
-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 = >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)
-- 
2.5.0