diff mbox series

[02/15] ppc: Add a return value to ppc_set_compat() and ppc_set_compat_all()

Message ID 20200914123505.612812-3-groug@kaod.org (mailing list archive)
State New, archived
Headers show
Series spapr: Error handling fixes and cleanups (round 2) | expand

Commit Message

Greg Kurz Sept. 14, 2020, 12:34 p.m. UTC 
As recommended in "qapi/error.h", indicate success / failure with a
return value. Since ppc_set_compat() is called from a VMState handler,
let's make it an int so that it propagates any negative errno returned
by kvmppc_set_compat(). Do the same for ppc_set_compat_all() for
consistency, even if it isn't called in a context where a negative errno
is required on failure.

This will allow to simplify error handling in the callers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/cpu.h    |  4 ++--
 target/ppc/compat.c | 26 +++++++++++++++-----------
 2 files changed, 17 insertions(+), 13 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Sept. 15, 2020, 9:18 a.m. UTC | #1 
14.09.2020 15:34, Greg Kurz wrote:
> As recommended in "qapi/error.h", indicate success / failure with a
> return value. Since ppc_set_compat() is called from a VMState handler,

What handler do you mean? You don't update any handlers here..

> let's make it an int so that it propagates any negative errno returned
> by kvmppc_set_compat(). Do the same for ppc_set_compat_all() for
> consistency, even if it isn't called in a context where a negative errno
> is required on failure.
> 
> This will allow to simplify error handling in the callers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

patch is OK:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Greg Kurz Sept. 15, 2020, 9:34 a.m. UTC | #2 
On Tue, 15 Sep 2020 12:18:35 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 14.09.2020 15:34, Greg Kurz wrote:
> > As recommended in "qapi/error.h", indicate success / failure with a
> > return value. Since ppc_set_compat() is called from a VMState handler,
> 
> What handler do you mean? You don't update any handlers here..
> 

One of the callers of ppc_set_compat() is

static int cpu_post_load(void *opaque, int version_id)
{

}

in target/ppc/machine.c, which gets fixed in patch 3. I mention this to
justify the choice of an int rather than a bool.

> > let's make it an int so that it propagates any negative errno returned
> > by kvmppc_set_compat(). Do the same for ppc_set_compat_all() for
> > consistency, even if it isn't called in a context where a negative errno
> > is required on failure.
> > 
> > This will allow to simplify error handling in the callers.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> patch is OK:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
diff mbox series

Patch

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 766e9c5c26fa..e8aa185d4ff8 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1352,10 +1352,10 @@  bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
 bool ppc_type_check_compat(const char *cputype, uint32_t compat_pvr,
                            uint32_t min_compat_pvr, uint32_t max_compat_pvr);
 
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
+int ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
 
 #if !defined(CONFIG_USER_ONLY)
-void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
+int ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
 #endif
 int ppc_compat_max_vthreads(PowerPCCPU *cpu);
 void ppc_compat_add_property(Object *obj, const char *name,
diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index 08aede88dc1d..e9bec5ffedbf 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -158,7 +158,7 @@  bool ppc_type_check_compat(const char *cputype, uint32_t compat_pvr,
     return pcc_compat(pcc, compat_pvr, min_compat_pvr, max_compat_pvr);
 }
 
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
+int ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
 {
     const CompatInfo *compat = compat_by_pvr(compat_pvr);
     CPUPPCState *env = &cpu->env;
@@ -169,11 +169,11 @@  void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
         pcr = 0;
     } else if (!compat) {
         error_setg(errp, "Unknown compatibility PVR 0x%08"PRIx32, compat_pvr);
-        return;
+        return -EINVAL;
     } else if (!ppc_check_compat(cpu, compat_pvr, 0, 0)) {
         error_setg(errp, "Compatibility PVR 0x%08"PRIx32" not valid for CPU",
                    compat_pvr);
-        return;
+        return -EINVAL;
     } else {
         pcr = compat->pcr;
     }
@@ -185,17 +185,19 @@  void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
         if (ret < 0) {
             error_setg_errno(errp, -ret,
                              "Unable to set CPU compatibility mode in KVM");
-            return;
+            return ret;
         }
     }
 
     cpu->compat_pvr = compat_pvr;
     env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
+    return 0;
 }
 
 typedef struct {
     uint32_t compat_pvr;
-    Error *err;
+    Error **errp;
+    int ret;
 } SetCompatState;
 
 static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
@@ -203,26 +205,28 @@  static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     SetCompatState *s = arg.host_ptr;
 
-    ppc_set_compat(cpu, s->compat_pvr, &s->err);
+    s->ret = ppc_set_compat(cpu, s->compat_pvr, s->errp);
 }
 
-void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
+int ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
 {
     CPUState *cs;
 
     CPU_FOREACH(cs) {
         SetCompatState s = {
             .compat_pvr = compat_pvr,
-            .err = NULL,
+            .errp = errp,
+            .ret = 0,
         };
 
         run_on_cpu(cs, do_set_compat, RUN_ON_CPU_HOST_PTR(&s));
 
-        if (s.err) {
-            error_propagate(errp, s.err);
-            return;
+        if (s.ret < 0) {
+            return s.ret;
         }
     }
+
+    return 0;
 }
 
 int ppc_compat_max_vthreads(PowerPCCPU *cpu)