[PATCH 00/15] spapr: Error handling fixes and cleanups (round 2)

[PATCH 00/15] spapr: Error handling fixes and cleanups (round 2)

[Greg Kurz]

Yet another round of sanitizing the error handling in spapr. I've
identified locations that needed fixing with the errp-guard.cocci
coccinelle script. It turns out that a better result is achieved
by fixing manually, especially by converting some void functions
to indicate success/failure with a return value.

Greg Kurz (15):
  spapr: Fix error leak in spapr_realize_vcpu()
  ppc: Add a return value to ppc_set_compat() and ppc_set_compat_all()
  ppc: Fix return value in cpu_post_load() error path
  spapr: Simplify error handling in callers of ppc_set_compat()
  spapr: Get rid of cas_check_pvr() error reporting
  spapr: Simplify error handling in do_client_architecture_support()
  spapr: Simplify error handling in spapr_vio_busdev_realize()
  spapr: Add a return value to spapr_drc_attach()
  spapr: Simplify error handling in prop_get_fdt()
  spapr: Add a return value to spapr_set_vcpu_id()
  spapr: Simplify error handling in spapr_cpu_core_realize()
  spapr: Add a return value to spapr_nvdimm_validate()
  spapr: Add a return value to spapr_check_pagesize()
  spapr: Simplify error handling in spapr_memory_plug()
  spapr: Simplify error handling in spapr_memory_unplug_request()

 include/hw/ppc/spapr.h        |  4 +-
 include/hw/ppc/spapr_drc.h    |  2 +-
 include/hw/ppc/spapr_nvdimm.h |  4 +-
 target/ppc/cpu.h              |  4 +-
 hw/ppc/spapr.c                | 76 ++++++++++++-----------------------
 hw/ppc/spapr_caps.c           |  7 +++-
 hw/ppc/spapr_cpu_core.c       | 24 +++++------
 hw/ppc/spapr_drc.c            | 17 ++++----
 hw/ppc/spapr_hcall.c          | 34 +++++++---------
 hw/ppc/spapr_nvdimm.c         | 24 +++++------
 hw/ppc/spapr_pci.c            |  5 +--
 hw/ppc/spapr_vio.c            | 12 +++---
 target/ppc/compat.c           | 26 +++++++-----
 target/ppc/machine.c          |  9 +++--
 14 files changed, 108 insertions(+), 140 deletions(-)

-- 
2.26.2

[PATCH 01/15] spapr: Fix error leak in spapr_realize_vcpu()

[Greg Kurz]

If spapr_irq_cpu_intc_create() fails, local_err isn't propagated and
thus leaked.

Fixes: 992861fb1e4c ("error: Eliminate error_propagate() manually")
Cc: armbru@redhat.com
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2125fdac348f..3e4f402b2e9f 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -232,7 +232,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
 {
     CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
-    Error *local_err = NULL;
 
     if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
         return;
@@ -244,7 +243,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
     cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
     kvmppc_set_papr(cpu);
 
-    if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
+    if (spapr_irq_cpu_intc_create(spapr, cpu, errp) < 0) {
         cpu_remove_sync(CPU(cpu));
         return;
     }
-- 
2.26.2

Re: [PATCH 01/15] spapr: Fix error leak in spapr_realize_vcpu()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> If spapr_irq_cpu_intc_create() fails, local_err isn't propagated and
> thus leaked.
> 
> Fixes: 992861fb1e4c ("error: Eliminate error_propagate() manually")
> Cc: armbru@redhat.com
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   hw/ppc/spapr_cpu_core.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2125fdac348f..3e4f402b2e9f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -232,7 +232,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>   {
>       CPUPPCState *env = &cpu->env;
>       CPUState *cs = CPU(cpu);
> -    Error *local_err = NULL;
>   
>       if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
>           return;
> @@ -244,7 +243,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>       kvmppc_set_papr(cpu);
>   
> -    if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
> +    if (spapr_irq_cpu_intc_create(spapr, cpu, errp) < 0) {
>           cpu_remove_sync(CPU(cpu));
>           return;
>       }
> 


-- 
Best regards,
Vladimir

Re: [PATCH 01/15] spapr: Fix error leak in spapr_realize_vcpu()

[Philippe Mathieu-Daudé]

On 9/14/20 2:34 PM, Greg Kurz wrote:
> If spapr_irq_cpu_intc_create() fails, local_err isn't propagated and
> thus leaked.
> 
> Fixes: 992861fb1e4c ("error: Eliminate error_propagate() manually")
> Cc: armbru@redhat.com
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_cpu_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Greg Kurz]

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

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

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

[Vladimir Sementsov-Ogievskiy]

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>

-- 
Best regards,
Vladimir

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

[Greg Kurz]

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

[PATCH 03/15] ppc: Fix return value in cpu_post_load() error path

[Greg Kurz]

VMState handlers are supposed to return negative errno values on failure.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/machine.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 109d0711628f..5e5837737679 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -347,18 +347,19 @@ static int cpu_post_load(void *opaque, int version_id)
     if (cpu->compat_pvr) {
         uint32_t compat_pvr = cpu->compat_pvr;
         Error *local_err = NULL;
+        int ret;
 
         cpu->compat_pvr = 0;
-        ppc_set_compat(cpu, compat_pvr, &local_err);
-        if (local_err) {
+        ret = ppc_set_compat(cpu, compat_pvr, &local_err);
+        if (ret < 0) {
             error_report_err(local_err);
-            return -1;
+            return ret;
         }
     } else
 #endif
     {
         if (!pvr_match(cpu, env->spr[SPR_PVR])) {
-            return -1;
+            return -EINVAL;
         }
     }
 
-- 
2.26.2

Re: [PATCH 03/15] ppc: Fix return value in cpu_post_load() error path

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> VMState handlers are supposed to return negative errno values on failure.

Even vmstate_load_state() itself hase both "return -EINVAL" and "return -1".. So, all it's a mess)

> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 03/15] ppc: Fix return value in cpu_post_load() error path

[Philippe Mathieu-Daudé]

On 9/14/20 2:34 PM, Greg Kurz wrote:
> VMState handlers are supposed to return negative errno values on failure.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  target/ppc/machine.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 04/15] spapr: Simplify error handling in callers of ppc_set_compat()

[Greg Kurz]

Now that ppc_set_compat() indicates success/failure with a return
value, use it and reduce error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ea2c755310cd..c0a3f5f26d97 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3820,10 +3820,9 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
      */
     if (hotplugged) {
         for (i = 0; i < cc->nr_threads; i++) {
-            ppc_set_compat(core->threads[i], POWERPC_CPU(first_cpu)->compat_pvr,
-                           &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
+            if (ppc_set_compat(core->threads[i],
+                               POWERPC_CPU(first_cpu)->compat_pvr,
+                               errp) < 0) {
                 return;
             }
         }
-- 
2.26.2

Re: [PATCH 04/15] spapr: Simplify error handling in callers of ppc_set_compat()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> Now that ppc_set_compat() indicates success/failure with a return
> value, use it and reduce error propagation overhead.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 04/15] spapr: Simplify error handling in callers of ppc_set_compat()

[Philippe Mathieu-Daudé]

On 9/14/20 2:34 PM, Greg Kurz wrote:
> Now that ppc_set_compat() indicates success/failure with a return
> value, use it and reduce error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 05/15] spapr: Get rid of cas_check_pvr() error reporting

[Greg Kurz]

The cas_check_pvr() function has two purposes:
- finding the "best" logical PVR, ie. the most recent one supported by
  the guest for this CPU type
- checking if the guest supports the real PVR of this CPU type, which
  is just an optional extra information to workaround the lack of
  support for "compat" mode in PR KVM

This logic doesn't need error reporting, really. If we don't find a
suitable logical PVR, we return the special value 0 which is definitely
not a valid PVR. Let the caller decide on whether it should error out
or not.

This doesn't change the behavior.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_hcall.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index c2776b6a7de2..885ea60778ba 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1590,12 +1590,11 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
     }
 }
 
-static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
-                              target_ulong *addr, bool *raw_mode_supported,
-                              Error **errp)
+/* Returns either a logical PVR or zero if none was found */
+static uint32_t cas_check_pvr(PowerPCCPU *cpu, uint32_t max_compat,
+                              target_ulong *addr, bool *raw_mode_supported)
 {
     bool explicit_match = false; /* Matched the CPU's real PVR */
-    uint32_t max_compat = spapr->max_compat_pvr;
     uint32_t best_compat = 0;
     int i;
 
@@ -1624,14 +1623,6 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
         }
     }
 
-    if ((best_compat == 0) && (!explicit_match || max_compat)) {
-        /* We couldn't find a suitable compatibility mode, and either
-         * the guest doesn't support "raw" mode for this CPU, or raw
-         * mode is disabled because a maximum compat mode is set */
-        error_setg(errp, "Couldn't negotiate a suitable PVR during CAS");
-        return 0;
-    }
-
     *raw_mode_supported = explicit_match;
 
     /* Parsing finished */
@@ -1680,6 +1671,7 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
     bool guest_xive;
     CPUState *cs;
     void *fdt;
+    uint32_t max_compat = spapr->max_compat_pvr;
 
     /* CAS is supposed to be called early when only the boot vCPU is active. */
     CPU_FOREACH(cs) {
@@ -1692,9 +1684,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
         }
     }
 
-    cas_pvr = cas_check_pvr(spapr, cpu, &vec, &raw_mode_supported, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
+    cas_pvr = cas_check_pvr(cpu, max_compat, &vec, &raw_mode_supported);
+    if (!cas_pvr && (!raw_mode_supported || max_compat)) {
+        /*
+         * We couldn't find a suitable compatibility mode, and either
+         * the guest doesn't support "raw" mode for this CPU, or "raw"
+         * mode is disabled because a maximum compat mode is set.
+         */
+        error_report("Couldn't negotiate a suitable PVR during CAS");
         return H_HARDWARE;
     }
 
-- 
2.26.2

Re: [PATCH 05/15] spapr: Get rid of cas_check_pvr() error reporting

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> The cas_check_pvr() function has two purposes:
> - finding the "best" logical PVR, ie. the most recent one supported by
>    the guest for this CPU type
> - checking if the guest supports the real PVR of this CPU type, which
>    is just an optional extra information to workaround the lack of
>    support for "compat" mode in PR KVM
> 
> This logic doesn't need error reporting, really. If we don't find a
> suitable logical PVR, we return the special value 0 which is definitely
> not a valid PVR. Let the caller decide on whether it should error out
> or not.

maybe then rename it s/cas_check_pvr/cas_find_compat_pvr/ or something like this?

> 
> This doesn't change the behavior.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

Re: [SPAM] Re: [PATCH 05/15] spapr: Get rid of cas_check_pvr() error reporting

[Greg Kurz]

On Tue, 15 Sep 2020 13:03:13 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 14.09.2020 15:34, Greg Kurz wrote:
> > The cas_check_pvr() function has two purposes:
> > - finding the "best" logical PVR, ie. the most recent one supported by
> >    the guest for this CPU type
> > - checking if the guest supports the real PVR of this CPU type, which
> >    is just an optional extra information to workaround the lack of
> >    support for "compat" mode in PR KVM
> > 
> > This logic doesn't need error reporting, really. If we don't find a
> > suitable logical PVR, we return the special value 0 which is definitely
> > not a valid PVR. Let the caller decide on whether it should error out
> > or not.
> 
> maybe then rename it s/cas_check_pvr/cas_find_compat_pvr/ or something like this?
> 

Ah yes, since this function doesn't do an actual check anymore, it may
be worth changing its name.

> > 
> > This doesn't change the behavior.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> 

[PATCH 06/15] spapr: Simplify error handling in do_client_architecture_support()

[Greg Kurz]

Use the return value of ppc_set_compat_all() to check failures,
which is preferred over hijacking local_err.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_hcall.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 885ea60778ba..607740150fa2 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1666,7 +1666,6 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
     uint32_t cas_pvr;
     SpaprOptionVector *ov1_guest, *ov5_guest;
     bool guest_radix;
-    Error *local_err = NULL;
     bool raw_mode_supported = false;
     bool guest_xive;
     CPUState *cs;
@@ -1697,8 +1696,9 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
 
     /* Update CPUs */
     if (cpu->compat_pvr != cas_pvr) {
-        ppc_set_compat_all(cas_pvr, &local_err);
-        if (local_err) {
+        Error *local_err = NULL;
+
+        if (ppc_set_compat_all(cas_pvr, &local_err) < 0) {
             /* We fail to set compat mode (likely because running with KVM PR),
              * but maybe we can fallback to raw mode if the guest supports it.
              */
@@ -1707,7 +1707,6 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
                 return H_HARDWARE;
             }
             error_free(local_err);
-            local_err = NULL;
         }
     }
 
-- 
2.26.2

Re: [PATCH 06/15] spapr: Simplify error handling in do_client_architecture_support()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> Use the return value of ppc_set_compat_all() to check failures,
> which is preferred over hijacking local_err.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 06/15] spapr: Simplify error handling in do_client_architecture_support()

[Philippe Mathieu-Daudé]

On 9/14/20 2:34 PM, Greg Kurz wrote:
> Use the return value of ppc_set_compat_all() to check failures,
> which is preferred over hijacking local_err.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_hcall.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 07/15] spapr: Simplify error handling in spapr_vio_busdev_realize()

[Greg Kurz]

Use the return value of spapr_irq_findone() and spapr_irq_claim()
to detect failures. This allows to reduce the error propagation
overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_vio.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 731080d989f1..44fdd64b88af 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -474,7 +474,6 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
     SpaprVioDevice *dev = (SpaprVioDevice *)qdev;
     SpaprVioDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
     char *id;
-    Error *local_err = NULL;
 
     if (dev->reg != -1) {
         /*
@@ -510,16 +509,15 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
     dev->irq = spapr_vio_reg_to_irq(dev->reg);
 
     if (SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
-        dev->irq = spapr_irq_findone(spapr, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        int irq = spapr_irq_findone(spapr, errp);
+
+        if (irq < 0) {
             return;
         }
+        dev->irq = irq;
     }
 
-    spapr_irq_claim(spapr, dev->irq, false, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (spapr_irq_claim(spapr, dev->irq, false, errp) < 0) {
         return;
     }
 
-- 
2.26.2

Re: [PATCH 07/15] spapr: Simplify error handling in spapr_vio_busdev_realize()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> Use the return value of spapr_irq_findone() and spapr_irq_claim()
> to detect failures. This allows to reduce the error propagation
> overhead.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 08/15] spapr: Add a return value to spapr_drc_attach()

[Greg Kurz]

As recommended in "qapi/error.h", return true on success and false on
failure. This allows to reduce error propagation overhead in the callers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr_drc.h |  2 +-
 hw/ppc/spapr.c             | 15 +++------------
 hw/ppc/spapr_drc.c         |  5 +++--
 hw/ppc/spapr_nvdimm.c      |  5 +----
 hw/ppc/spapr_pci.c         |  5 +----
 5 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index f2708607696e..165b281496bb 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -235,7 +235,7 @@ SpaprDrc *spapr_drc_by_index(uint32_t index);
 SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
 int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t drc_type_mask);
 
-void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
+bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp);
 void spapr_drc_detach(SpaprDrc *drc);
 
 /* Returns true if a hot plug/unplug request is pending */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c0a3f5f26d97..8b2b4e6272e6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3374,22 +3374,19 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
     int i;
     uint64_t addr = addr_start;
     bool hotplugged = spapr_drc_hotplugged(dev);
-    Error *local_err = NULL;
 
     for (i = 0; i < nr_lmbs; i++) {
         drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
                               addr / SPAPR_MEMORY_BLOCK_SIZE);
         g_assert(drc);
 
-        spapr_drc_attach(drc, dev, &local_err);
-        if (local_err) {
+        if (!spapr_drc_attach(drc, dev, errp)) {
             while (addr > addr_start) {
                 addr -= SPAPR_MEMORY_BLOCK_SIZE;
                 drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
                                       addr / SPAPR_MEMORY_BLOCK_SIZE);
                 spapr_drc_detach(drc);
             }
-            error_propagate(errp, local_err);
             return;
         }
         if (!hotplugged) {
@@ -3770,7 +3767,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     CPUCore *cc = CPU_CORE(dev);
     CPUState *cs;
     SpaprDrc *drc;
-    Error *local_err = NULL;
     CPUArchId *core_slot;
     int index;
     bool hotplugged = spapr_drc_hotplugged(dev);
@@ -3788,9 +3784,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     g_assert(drc || !mc->has_hotpluggable_cpus);
 
     if (drc) {
-        spapr_drc_attach(drc, dev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!spapr_drc_attach(drc, dev, errp)) {
             return;
         }
 
@@ -3942,7 +3936,6 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
-    Error *local_err = NULL;
 
     if (!smc->dr_phb_enabled) {
         return;
@@ -3952,9 +3945,7 @@ static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     /* hotplug hooks should check it's enabled before getting this far */
     assert(drc);
 
-    spapr_drc_attach(drc, dev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!spapr_drc_attach(drc, dev, errp)) {
         return;
     }
 
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fe998d8108b1..04a6bf134ee8 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -371,13 +371,13 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
     } while (fdt_depth != 0);
 }
 
-void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
+bool spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
 {
     trace_spapr_drc_attach(spapr_drc_index(drc));
 
     if (drc->dev) {
         error_setg(errp, "an attached device is still awaiting release");
-        return;
+        return false;
     }
     g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
              || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
@@ -388,6 +388,7 @@ void spapr_drc_attach(SpaprDrc *drc, DeviceState *d, Error **errp)
                              object_get_typename(OBJECT(drc->dev)),
                              (Object **)(&drc->dev),
                              NULL, 0);
+    return true;
 }
 
 static void spapr_drc_release(SpaprDrc *drc)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 63872054f375..c06f903e5bff 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -91,14 +91,11 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
 {
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
-    Error *local_err = NULL;
 
     drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PMEM, slot);
     g_assert(drc);
 
-    spapr_drc_attach(drc, dev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!spapr_drc_attach(drc, dev, errp)) {
         return;
     }
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4d97ff6c70f2..30d054a4d04c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1539,7 +1539,6 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(plugged_dev);
     SpaprDrc *drc = drc_from_dev(phb, pdev);
-    Error *local_err = NULL;
     PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
     uint32_t slotnr = PCI_SLOT(pdev->devfn);
 
@@ -1578,9 +1577,7 @@ static void spapr_pci_plug(HotplugHandler *plug_handler,
         return;
     }
 
-    spapr_drc_attach(drc, DEVICE(pdev), &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!spapr_drc_attach(drc, DEVICE(pdev), errp)) {
         return;
     }
 
-- 
2.26.2

Re: [PATCH 08/15] spapr: Add a return value to spapr_drc_attach()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 08/15] spapr: Add a return value to spapr_drc_attach()

[Philippe Mathieu-Daudé]

On 9/14/20 2:34 PM, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr_drc.h |  2 +-
>  hw/ppc/spapr.c             | 15 +++------------
>  hw/ppc/spapr_drc.c         |  5 +++--
>  hw/ppc/spapr_nvdimm.c      |  5 +----
>  hw/ppc/spapr_pci.c         |  5 +----
>  5 files changed, 9 insertions(+), 23 deletions(-)

Good cleanup.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 09/15] spapr: Simplify error handling in prop_get_fdt()

[Greg Kurz]

Use the return value of visit_check_struct() and visit_check_list()
for error checking instead of local_err. This allows to get rid of
the error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_drc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 04a6bf134ee8..697b28c34305 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -302,7 +302,6 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
 {
     SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
     QNull *null = NULL;
-    Error *err = NULL;
     int fdt_offset_next, fdt_offset, fdt_depth;
     void *fdt;
 
@@ -321,6 +320,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
         const struct fdt_property *prop = NULL;
         int prop_len = 0, name_len = 0;
         uint32_t tag;
+        bool ok;
 
         tag = fdt_next_tag(fdt, fdt_offset, &fdt_offset_next);
         switch (tag) {
@@ -334,10 +334,9 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
         case FDT_END_NODE:
             /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
             g_assert(fdt_depth > 0);
-            visit_check_struct(v, &err);
+            ok = visit_check_struct(v, errp);
             visit_end_struct(v, NULL);
-            if (err) {
-                error_propagate(errp, err);
+            if (!ok) {
                 return;
             }
             fdt_depth--;
@@ -355,10 +354,9 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
                     return;
                 }
             }
-            visit_check_list(v, &err);
+            ok = visit_check_list(v, errp);
             visit_end_list(v, NULL);
-            if (err) {
-                error_propagate(errp, err);
+            if (!ok) {
                 return;
             }
             break;
-- 
2.26.2

Re: [PATCH 09/15] spapr: Simplify error handling in prop_get_fdt()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:34, Greg Kurz wrote:
> Use the return value of visit_check_struct() and visit_check_list()
> for error checking instead of local_err. This allows to get rid of
> the error propagation overhead.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id()

[Greg Kurz]

As recommended in "qapi/error.h", return true on success and false on
failure. This allows to reduce error propagation overhead in the callers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr.h  | 2 +-
 hw/ppc/spapr.c          | 5 +++--
 hw/ppc/spapr_cpu_core.c | 5 +----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c8cd63bc0667..11682f00e8cc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -909,7 +909,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
 int spapr_get_vcpu_id(PowerPCCPU *cpu);
-void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
+bool spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
 PowerPCCPU *spapr_find_cpu(int vcpu_id);
 
 int spapr_caps_pre_load(void *opaque);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8b2b4e6272e6..e11472a53ab4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4289,7 +4289,7 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu)
     return cpu->vcpu_id;
 }
 
-void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
+bool spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     MachineState *ms = MACHINE(spapr);
@@ -4302,10 +4302,11 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
         error_append_hint(errp, "Adjust the number of cpus to %d "
                           "or try to raise the number of threads per core\n",
                           vcpu_id * ms->smp.threads / spapr->vsmt);
-        return;
+        return false;
     }
 
     cpu->vcpu_id = vcpu_id;
+    return true;
 }
 
 PowerPCCPU *spapr_find_cpu(int vcpu_id)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4f402b2e9f..0c879d4da262 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -262,7 +262,6 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
     char *id;
     CPUState *cs;
     PowerPCCPU *cpu;
-    Error *local_err = NULL;
 
     obj = object_new(scc->cpu_type);
 
@@ -274,8 +273,7 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
      */
     cs->start_powered_off = true;
     cs->cpu_index = cc->core_id + i;
-    spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
-    if (local_err) {
+    if (!spapr_set_vcpu_id(cpu, cs->cpu_index, errp)) {
         goto err;
     }
 
@@ -292,7 +290,6 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
 
 err:
     object_unref(obj);
-    error_propagate(errp, local_err);
     return NULL;
 }
 
-- 
2.26.2

Re: [PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:35, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id()

[Philippe Mathieu-Daudé]

On 9/14/20 2:35 PM, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr.h  | 2 +-
>  hw/ppc/spapr.c          | 5 +++--
>  hw/ppc/spapr_cpu_core.c | 5 +----
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c8cd63bc0667..11682f00e8cc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -909,7 +909,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>  
>  int spapr_get_vcpu_id(PowerPCCPU *cpu);
> -void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
> +bool spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);

If you have to respin, please add some doc, at least this would
be an improvement:

/* Returns: %true on success, %false on error. */

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id()

[Greg Kurz]

On Tue, 15 Sep 2020 15:08:05 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 9/14/20 2:35 PM, Greg Kurz wrote:
> > As recommended in "qapi/error.h", return true on success and false on
> > failure. This allows to reduce error propagation overhead in the callers.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  include/hw/ppc/spapr.h  | 2 +-
> >  hw/ppc/spapr.c          | 5 +++--
> >  hw/ppc/spapr_cpu_core.c | 5 +----
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c8cd63bc0667..11682f00e8cc 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -909,7 +909,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> >  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
> >  
> >  int spapr_get_vcpu_id(PowerPCCPU *cpu);
> > -void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
> > +bool spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
> 
> If you have to respin, please add some doc, at least this would
> be an improvement:
> 
> /* Returns: %true on success, %false on error. */
> 

Yeah, most, not to say all, APIs in the spapr code don't have
doc in the header files... which uselessly forces everyone to
check what the function actually does. Not sure how to best
address that though.

Adding headers everywhere (ie. lot of churn) ? Only in selected places
where it isn't obvious ? Also for functions that return integers or
pointers ?

I'll cowardly let David decide ;-)

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Re: [PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id()

[David Gibson]

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

On Tue, Sep 15, 2020 at 03:53:52PM +0200, Greg Kurz wrote:
> On Tue, 15 Sep 2020 15:08:05 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > On 9/14/20 2:35 PM, Greg Kurz wrote:
> > > As recommended in "qapi/error.h", return true on success and false on
> > > failure. This allows to reduce error propagation overhead in the callers.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  include/hw/ppc/spapr.h  | 2 +-
> > >  hw/ppc/spapr.c          | 5 +++--
> > >  hw/ppc/spapr_cpu_core.c | 5 +----
> > >  3 files changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index c8cd63bc0667..11682f00e8cc 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -909,7 +909,7 @@ void spapr_do_system_reset_on_cpu(CPUState *cs, run_on_cpu_data arg);
> > >  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
> > >  
> > >  int spapr_get_vcpu_id(PowerPCCPU *cpu);
> > > -void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
> > > +bool spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
> > 
> > If you have to respin, please add some doc, at least this would
> > be an improvement:
> > 
> > /* Returns: %true on success, %false on error. */
> > 
> 
> Yeah, most, not to say all, APIs in the spapr code don't have
> doc in the header files... which uselessly forces everyone to
> check what the function actually does. Not sure how to best
> address that though.
> 
> Adding headers everywhere (ie. lot of churn) ? Only in selected places
> where it isn't obvious ? Also for functions that return integers or
> pointers ?
> 
> I'll cowardly let David decide ;-)

And I'll lazily reply that I'm happy to take patches adding
documentation, but I'm not going to undertake a big effort to add it
comprehensively.

-- 
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: 833 bytes --]

[PATCH 11/15] spapr: Simplify error handling in spapr_cpu_core_realize()

[Greg Kurz]

As recommended in "qapi/error.h", add a bool return value to
spapr_realize_vcpu() and use it in spapr_cpu_core_realize()
in order to get rid of the error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 0c879d4da262..b03620823adb 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -227,14 +227,14 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
     g_free(sc->threads);
 }
 
-static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
+static bool spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
                                SpaprCpuCore *sc, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
 
     if (!qdev_realize(DEVICE(cpu), NULL, errp)) {
-        return;
+        return false;
     }
 
     /* Set time-base frequency to 512 MHz */
@@ -245,13 +245,14 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
     if (spapr_irq_cpu_intc_create(spapr, cpu, errp) < 0) {
         cpu_remove_sync(CPU(cpu));
-        return;
+        return false;
     }
 
     if (!sc->pre_3_0_migration) {
         vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
                          cpu->machine_data);
     }
+    return true;
 }
 
 static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
@@ -312,7 +313,6 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
                                                   TYPE_SPAPR_MACHINE);
     SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
-    Error *local_err = NULL;
     int i, j;
 
     if (!spapr) {
@@ -322,15 +322,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
     sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
-        if (local_err) {
+        sc->threads[i] = spapr_create_vcpu(sc, i, errp);
+        if (!sc->threads[i]) {
             goto err;
         }
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        spapr_realize_vcpu(sc->threads[j], spapr, sc, &local_err);
-        if (local_err) {
+        if (!spapr_realize_vcpu(sc->threads[j], spapr, sc, errp)) {
             goto err_unrealize;
         }
     }
@@ -347,7 +346,6 @@ err:
         spapr_delete_vcpu(sc->threads[i], sc);
     }
     g_free(sc->threads);
-    error_propagate(errp, local_err);
 }
 
 static Property spapr_cpu_core_properties[] = {
-- 
2.26.2

Re: [PATCH 11/15] spapr: Simplify error handling in spapr_cpu_core_realize()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:35, Greg Kurz wrote:
> As recommended in "qapi/error.h", add a bool return value to
> spapr_realize_vcpu() and use it in spapr_cpu_core_realize()
> in order to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 11/15] spapr: Simplify error handling in spapr_cpu_core_realize()

[Philippe Mathieu-Daudé]

On 9/14/20 2:35 PM, Greg Kurz wrote:
> As recommended in "qapi/error.h", add a bool return value to
> spapr_realize_vcpu() and use it in spapr_cpu_core_realize()
> in order to get rid of the error propagation overhead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_cpu_core.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 12/15] spapr: Add a return value to spapr_nvdimm_validate()

[Greg Kurz]

As recommended in "qapi/error.h", return true on success and false on
failure. This allows to reduce error propagation overhead in the callers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr_nvdimm.h |  2 +-
 hw/ppc/spapr.c                |  4 +---
 hw/ppc/spapr_nvdimm.c         | 14 ++++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index 3eb344e8e976..b834d82f5545 100644
--- a/include/hw/ppc/spapr_nvdimm.h
+++ b/include/hw/ppc/spapr_nvdimm.h
@@ -28,7 +28,7 @@ QEMU_BUILD_BUG_ON(SPAPR_MINIMUM_SCM_BLOCK_SIZE % SPAPR_MEMORY_BLOCK_SIZE);
 int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
                            void *fdt, int *fdt_start_offset, Error **errp);
 void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
-void spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
+bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
                            uint64_t size, Error **errp);
 void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
 void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e11472a53ab4..bfa2a00da77e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3481,9 +3481,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 
     if (is_nvdimm) {
-        spapr_nvdimm_validate(hotplug_dev, NVDIMM(dev), size, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!spapr_nvdimm_validate(hotplug_dev, NVDIMM(dev), size, errp)) {
             return;
         }
     } else if (size % SPAPR_MEMORY_BLOCK_SIZE) {
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index c06f903e5bff..b3a489e9fe18 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -33,7 +33,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/ppc/spapr_numa.h"
 
-void spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
+bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
                            uint64_t size, Error **errp)
 {
     const MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
@@ -45,7 +45,7 @@ void spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 
     if (!mc->nvdimm_supported) {
         error_setg(errp, "NVDIMM hotplug not supported for this machine");
-        return;
+        return false;
     }
 
     /*
@@ -59,20 +59,20 @@ void spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
      */
     if (!ms->nvdimms_state->is_enabled && nvdimm_opt) {
         error_setg(errp, "nvdimm device found but 'nvdimm=off' was set");
-        return;
+        return false;
     }
 
     if (object_property_get_int(OBJECT(nvdimm), NVDIMM_LABEL_SIZE_PROP,
                                 &error_abort) == 0) {
         error_setg(errp, "PAPR requires NVDIMM devices to have label-size set");
-        return;
+        return false;
     }
 
     if (size % SPAPR_MINIMUM_SCM_BLOCK_SIZE) {
         error_setg(errp, "PAPR requires NVDIMM memory size (excluding label)"
                    " to be a multiple of %" PRIu64 "MB",
                    SPAPR_MINIMUM_SCM_BLOCK_SIZE / MiB);
-        return;
+        return false;
     }
 
     uuidstr = object_property_get_str(OBJECT(nvdimm), NVDIMM_UUID_PROP,
@@ -82,8 +82,10 @@ void spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 
     if (qemu_uuid_is_null(&uuid)) {
         error_setg(errp, "NVDIMM device requires the uuid to be set");
-        return;
+        return false;
     }
+
+    return true;
 }
 
 
-- 
2.26.2

Re: [PATCH 12/15] spapr: Add a return value to spapr_nvdimm_validate()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:35, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 12/15] spapr: Add a return value to spapr_nvdimm_validate()

[Philippe Mathieu-Daudé]

On 9/14/20 2:35 PM, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr_nvdimm.h |  2 +-
>  hw/ppc/spapr.c                |  4 +---
>  hw/ppc/spapr_nvdimm.c         | 14 ++++++++------
>  3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 13/15] spapr: Add a return value to spapr_check_pagesize()

[Greg Kurz]

As recommended in "qapi/error.h", return true on success and false on
failure. This allows to reduce error propagation overhead in the callers.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr.h | 2 +-
 hw/ppc/spapr.c         | 4 +---
 hw/ppc/spapr_caps.c    | 7 +++++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 11682f00e8cc..114e81996988 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -941,7 +941,7 @@ void spapr_caps_cpu_apply(SpaprMachineState *spapr, PowerPCCPU *cpu);
 void spapr_caps_add_properties(SpaprMachineClass *smc);
 int spapr_caps_post_migration(SpaprMachineState *spapr);
 
-void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
+bool spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
                           Error **errp);
 /*
  * XIVE definitions
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bfa2a00da77e..e813c7cfb949 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3493,9 +3493,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
                                       &error_abort);
     pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
-    spapr_check_pagesize(spapr, pagesize, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!spapr_check_pagesize(spapr, pagesize, errp)) {
         return;
     }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 10a80a8159f4..9341e9782a3f 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -310,13 +310,13 @@ static void cap_safe_indirect_branch_apply(SpaprMachineState *spapr,
 
 #define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
 
-void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
+bool spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
                           Error **errp)
 {
     hwaddr maxpagesize = (1ULL << spapr->eff.caps[SPAPR_CAP_HPT_MAXPAGESIZE]);
 
     if (!kvmppc_hpt_needs_host_contiguous_pages()) {
-        return;
+        return true;
     }
 
     if (maxpagesize > pagesize) {
@@ -324,7 +324,10 @@ void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
                    "Can't support %"HWADDR_PRIu" kiB guest pages with %"
                    HWADDR_PRIu" kiB host pages with this KVM implementation",
                    maxpagesize >> 10, pagesize >> 10);
+        return false;
     }
+
+    return true;
 }
 
 static void cap_hpt_maxpagesize_apply(SpaprMachineState *spapr,
-- 
2.26.2

Re: [PATCH 13/15] spapr: Add a return value to spapr_check_pagesize()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:35, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz<groug@kaod.org>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 13/15] spapr: Add a return value to spapr_check_pagesize()

[Philippe Mathieu-Daudé]

On 9/14/20 2:35 PM, Greg Kurz wrote:
> As recommended in "qapi/error.h", return true on success and false on
> failure. This allows to reduce error propagation overhead in the callers.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr.h | 2 +-
>  hw/ppc/spapr.c         | 4 +---
>  hw/ppc/spapr_caps.c    | 7 +++++--
>  3 files changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

As recommended in "qapi/error.h", add a bool return value to
spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
of local_err in spapr_memory_plug().

Since object_property_get_uint() only returns 0 on failure, use
that as well.

Also call ERRP_GUARD() to be able to check the status of void
function pc_dimm_plug() with *errp.

This allows to get rid of the error propagation overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr_nvdimm.h |  2 +-
 hw/ppc/spapr.c                | 35 +++++++++++++----------------------
 hw/ppc/spapr_nvdimm.c         |  5 +++--
 3 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
index b834d82f5545..dc30edece997 100644
--- a/include/hw/ppc/spapr_nvdimm.h
+++ b/include/hw/ppc/spapr_nvdimm.h
@@ -30,7 +30,7 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
 void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt);
 bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
                            uint64_t size, Error **errp);
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
 void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr);
 
 #endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e813c7cfb949..d6b4de6a1c53 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3366,7 +3366,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     return 0;
 }
 
-static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
+static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                            bool dedicated_hp_event_source, Error **errp)
 {
     SpaprDrc *drc;
@@ -3387,7 +3387,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                       addr / SPAPR_MEMORY_BLOCK_SIZE);
                 spapr_drc_detach(drc);
             }
-            return;
+            return false;
         }
         if (!hotplugged) {
             spapr_drc_reset(drc);
@@ -3409,12 +3409,13 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                            nr_lmbs);
         }
     }
+    return true;
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_GUARD();
     SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint64_t size, addr, slot;
@@ -3422,39 +3423,29 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
 
-    pc_dimm_plug(dimm, MACHINE(ms), &local_err);
-    if (local_err) {
-        goto out;
+    pc_dimm_plug(dimm, MACHINE(ms), errp);
+    if (*errp) {
+        return;
     }
 
     if (!is_nvdimm) {
-        addr = object_property_get_uint(OBJECT(dimm),
-                                        PC_DIMM_ADDR_PROP, &local_err);
-        if (local_err) {
+        addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, errp);
+        if (!addr ||
+            !spapr_add_lmbs(dev, addr, size,
+                            spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) {
             goto out_unplug;
         }
-        spapr_add_lmbs(dev, addr, size,
-                       spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
-                       &local_err);
     } else {
-        slot = object_property_get_uint(OBJECT(dimm),
-                                        PC_DIMM_SLOT_PROP, &local_err);
-        if (local_err) {
+        slot = object_property_get_uint(OBJECT(dimm), PC_DIMM_SLOT_PROP, errp);
+        if (!slot || !spapr_add_nvdimm(dev, slot, errp)) {
             goto out_unplug;
         }
-        spapr_add_nvdimm(dev, slot, &local_err);
-    }
-
-    if (local_err) {
-        goto out_unplug;
     }
 
     return;
 
 out_unplug:
     pc_dimm_unplug(dimm, MACHINE(ms));
-out:
-    error_propagate(errp, local_err);
 }
 
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index b3a489e9fe18..361ac8c28e33 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 }
 
 
-void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
+bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
 {
     SpaprDrc *drc;
     bool hotplugged = spapr_drc_hotplugged(dev);
@@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp)
     g_assert(drc);
 
     if (!spapr_drc_attach(drc, dev, errp)) {
-        return;
+        return false;
     }
 
     if (hotplugged) {
         spapr_hotplug_req_add_by_index(drc);
     }
+    return true;
 }
 
 void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
-- 
2.26.2

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Vladimir Sementsov-Ogievskiy]

14.09.2020 15:35, Greg Kurz wrote:
> As recommended in "qapi/error.h", add a bool return value to
> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> of local_err in spapr_memory_plug().
> 
> Since object_property_get_uint() only returns 0 on failure, use
> that as well.

Why are you sure? Can't the property be 0 itself?

> 
> Also call ERRP_GUARD() to be able to check the status of void
> function pc_dimm_plug() with *errp.
> 


-- 
Best regards,
Vladimir

Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

On Tue, 15 Sep 2020 13:58:53 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 14.09.2020 15:35, Greg Kurz wrote:
> > As recommended in "qapi/error.h", add a bool return value to
> > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> > of local_err in spapr_memory_plug().
> > 
> > Since object_property_get_uint() only returns 0 on failure, use
> > that as well.
> 
> Why are you sure? Can't the property be 0 itself?
> 

Hmmm... I've based this assumption on the header:

 * Returns: the value of the property, converted to an unsigned integer, or 0
 * an error occurs (including when the property value is not an integer).

but looking at the implementation, I don't see any check that
a property cannot be 0 indeed...

It's a bit misleading to mention this in the header though. I
understand that the function should return something, which
should have a some explicitly assigned value to avoid compilers
or static analyzers to yell, but the written contract should be
that the value is _undefined_ IMHO.

In its present form, the only way to know if the property is
valid is to pass a non-NULL errp actually. I'd rather even see
that in the contract, and an assert() in the code.

An alternative would be to convert it to have a bool return
value and get the actual uint result with a pointer argument.

> > 
> > Also call ERRP_GUARD() to be able to check the status of void
> > function pc_dimm_plug() with *errp.
> > 
> 
> 

I'm now hesitating to either check *errp for object_property_get_uint()
too or simply drop this patch...

Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Vladimir Sementsov-Ogievskiy]

15.09.2020 14:43, Greg Kurz wrote:
> On Tue, 15 Sep 2020 13:58:53 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 14.09.2020 15:35, Greg Kurz wrote:
>>> As recommended in "qapi/error.h", add a bool return value to
>>> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
>>> of local_err in spapr_memory_plug().
>>>
>>> Since object_property_get_uint() only returns 0 on failure, use
>>> that as well.
>>
>> Why are you sure? Can't the property be 0 itself?
>>
> 
> Hmmm... I've based this assumption on the header:
> 
>   * Returns: the value of the property, converted to an unsigned integer, or 0
>   * an error occurs (including when the property value is not an integer).
> 
> but looking at the implementation, I don't see any check that
> a property cannot be 0 indeed...
> 
> It's a bit misleading to mention this in the header though. I
> understand that the function should return something, which
> should have a some explicitly assigned value to avoid compilers
> or static analyzers to yell, but the written contract should be
> that the value is _undefined_ IMHO.
> 
> In its present form, the only way to know if the property is
> valid is to pass a non-NULL errp actually. I'd rather even see
> that in the contract, and an assert() in the code.
> 
> An alternative would be to convert it to have a bool return
> value and get the actual uint result with a pointer argument.
> 
>>>
>>> Also call ERRP_GUARD() to be able to check the status of void
>>> function pc_dimm_plug() with *errp.
>>>
>>
>>
> 
> I'm now hesitating to either check *errp for object_property_get_uint()
> too or simply drop this patch...
> 

.. and the following. If no more reviewers come to look at it, you can just merge first 13 patches, not resending the whole series for last two patches, which may be moved to round 3.

-- 
Best regards,
Vladimir

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

I've dropped the SPAM mentions from the subject this time :)

On Tue, 15 Sep 2020 14:53:53 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 15.09.2020 14:43, Greg Kurz wrote:
> > On Tue, 15 Sep 2020 13:58:53 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> > 
> >> 14.09.2020 15:35, Greg Kurz wrote:
> >>> As recommended in "qapi/error.h", add a bool return value to
> >>> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> >>> of local_err in spapr_memory_plug().
> >>>
> >>> Since object_property_get_uint() only returns 0 on failure, use
> >>> that as well.
> >>
> >> Why are you sure? Can't the property be 0 itself?
> >>
> > 
> > Hmmm... I've based this assumption on the header:
> > 
> >   * Returns: the value of the property, converted to an unsigned integer, or 0
> >   * an error occurs (including when the property value is not an integer).
> > 
> > but looking at the implementation, I don't see any check that
> > a property cannot be 0 indeed...
> > 
> > It's a bit misleading to mention this in the header though. I
> > understand that the function should return something, which
> > should have a some explicitly assigned value to avoid compilers
> > or static analyzers to yell, but the written contract should be
> > that the value is _undefined_ IMHO.
> > 
> > In its present form, the only way to know if the property is
> > valid is to pass a non-NULL errp actually. I'd rather even see
> > that in the contract, and an assert() in the code.
> > 
> > An alternative would be to convert it to have a bool return
> > value and get the actual uint result with a pointer argument.
> > 
> >>>
> >>> Also call ERRP_GUARD() to be able to check the status of void
> >>> function pc_dimm_plug() with *errp.
> >>>
> >>
> >>
> > 
> > I'm now hesitating to either check *errp for object_property_get_uint()
> > too or simply drop this patch...
> > 
> 
> .. and the following. If no more reviewers come to look at it, you can just merge first 13 patches, not resending the whole series for last two patches, which may be moved to round 3.
> 

I don't expect much people except David or maybe Markus to look
at these patches actually. And anyway, it's up to David to merge
them. But, yes, I agree patch 14 and 15 should go to the next
round.

Thanks for the review !

Cheers,

--
Greg

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

On Tue, 15 Sep 2020 14:04:23 +0200
Greg Kurz <groug@kaod.org> wrote:
> 
> I don't expect much people except David or maybe Markus to look
> at these patches actually. And anyway, it's up to David to merge

My bad, I didn't think about Philippe... :P

Thanks for the review, Philippe !

Cheers,

--
Greg

> them. But, yes, I agree patch 14 and 15 should go to the next
> round.
> 
> Thanks for the review !
> 
> Cheers,
> 
> --
> Greg
> 

Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[David Gibson]

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

On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote:
> On Tue, 15 Sep 2020 13:58:53 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> > 14.09.2020 15:35, Greg Kurz wrote:
> > > As recommended in "qapi/error.h", add a bool return value to
> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> > > of local_err in spapr_memory_plug().
> > > 
> > > Since object_property_get_uint() only returns 0 on failure, use
> > > that as well.
> > 
> > Why are you sure? Can't the property be 0 itself?
> > 
> 
> Hmmm... I've based this assumption on the header:
> 
>  * Returns: the value of the property, converted to an unsigned integer, or 0
>  * an error occurs (including when the property value is not an integer).
> 
> but looking at the implementation, I don't see any check that
> a property cannot be 0 indeed...

Yeah, indeed I'm pretty sure it can.

> It's a bit misleading to mention this in the header though. I
> understand that the function should return something, which
> should have a some explicitly assigned value to avoid compilers
> or static analyzers to yell, but the written contract should be
> that the value is _undefined_ IMHO.

Hrm... I think the description could be clearer, but returning 0
explicitly on the failure case has some benefits too.  If 0 is a
reasonable default for when the property isn't present (which is a
plausibly common case) then it means you can just get a value and
ignore errors.

> In its present form, the only way to know if the property is
> valid is to pass a non-NULL errp actually. I'd rather even see
> that in the contract, and an assert() in the code.

Maybe... see above.

> An alternative would be to convert it to have a bool return
> value and get the actual uint result with a pointer argument.

I don't think this is a good idea.  Returning success/failure as the
return value is a good rule of thumb because it reduces the amount of
checking of out-of-band information you need to do.  If you move to
returning the actual value you're trying to get out of band in this
sense, it kind of defeats that purpose.

I think this one is a case where it is reasonable to make it required
to explicitly check the error value.

> > > Also call ERRP_GUARD() to be able to check the status of void
> > > function pc_dimm_plug() with *errp.
> 
> I'm now hesitating to either check *errp for object_property_get_uint()
> too or simply drop this patch...

-- 
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: 833 bytes --]

Re: [SPAM] Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote:
>> On Tue, 15 Sep 2020 13:58:53 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 
>> > 14.09.2020 15:35, Greg Kurz wrote:
>> > > As recommended in "qapi/error.h", add a bool return value to
>> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
>> > > of local_err in spapr_memory_plug().
>> > > 
>> > > Since object_property_get_uint() only returns 0 on failure, use
>> > > that as well.
>> > 
>> > Why are you sure? Can't the property be 0 itself?
>> > 
>> 
>> Hmmm... I've based this assumption on the header:
>> 
>>  * Returns: the value of the property, converted to an unsigned integer, or 0
>>  * an error occurs (including when the property value is not an integer).
>> 
>> but looking at the implementation, I don't see any check that
>> a property cannot be 0 indeed...
>
> Yeah, indeed I'm pretty sure it can.

Correct.

Corollary: you can't use to return value to check for failure, except
when you know the property cannot be zero (you commonly don't).

The function predates our (rather late) realization that returning a
recognizable error value in addition to setting an error leads to more
readable code.  Today, we'd perhaps do it the way you describe below.

>> It's a bit misleading to mention this in the header though. I
>> understand that the function should return something, which
>> should have a some explicitly assigned value to avoid compilers
>> or static analyzers to yell, but the written contract should be
>> that the value is _undefined_ IMHO.
>
> Hrm... I think the description could be clearer, but returning 0
> explicitly on the failure case has some benefits too.  If 0 is a
> reasonable default for when the property isn't present (which is a
> plausibly common case) then it means you can just get a value and
> ignore errors.

Matter of taste.

There's no shortage of _undefined_ in C...

>> In its present form, the only way to know if the property is
>> valid is to pass a non-NULL errp actually. I'd rather even see
>> that in the contract, and an assert() in the code.
>
> Maybe... see above.

If you think the contract could be improved, please post a patch.

What assertion do you have in mind?  If it's adding assert(errp) to
object_property_get_uint(), I'll object.  Functions should not place
additional restrictions on @errp arguments without a compelling reason.

>> An alternative would be to convert it to have a bool return
>> value and get the actual uint result with a pointer argument.
>
> I don't think this is a good idea.  Returning success/failure as the
> return value is a good rule of thumb because it reduces the amount of
> checking of out-of-band information you need to do.  If you move to
> returning the actual value you're trying to get out of band in this
> sense, it kind of defeats that purpose.
>
> I think this one is a case where it is reasonable to make it required
> to explicitly check the error value.

If almost all calls assign the value to a variable, like

    val = object_property_get_uint(obj, name, &err);
    if (err) {
        error_propagate(errp, err)
        ... bail out ...
    }
    ... use val ...

then the alternative Greg proposed is easier on the eyes:

    if (!object_property_get_uint(obj, name, &val, errp)) {
        ... bail out ...
    }
    ... use val ...

It isn't for calls that use the value without storing it in a variable
first.

>> > > Also call ERRP_GUARD() to be able to check the status of void
>> > > function pc_dimm_plug() with *errp.
>> 
>> I'm now hesitating to either check *errp for object_property_get_uint()
>> too or simply drop this patch...

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

On Thu, 17 Sep 2020 09:38:49 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote:
> >> On Tue, 15 Sep 2020 13:58:53 +0300
> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >> 
> >> > 14.09.2020 15:35, Greg Kurz wrote:
> >> > > As recommended in "qapi/error.h", add a bool return value to
> >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> >> > > of local_err in spapr_memory_plug().
> >> > > 
> >> > > Since object_property_get_uint() only returns 0 on failure, use
> >> > > that as well.
> >> > 
> >> > Why are you sure? Can't the property be 0 itself?
> >> > 
> >> 
> >> Hmmm... I've based this assumption on the header:
> >> 
> >>  * Returns: the value of the property, converted to an unsigned integer, or 0
> >>  * an error occurs (including when the property value is not an integer).
> >> 
> >> but looking at the implementation, I don't see any check that
> >> a property cannot be 0 indeed...
> >
> > Yeah, indeed I'm pretty sure it can.
> 
> Correct.
> 
> Corollary: you can't use to return value to check for failure, except
> when you know the property cannot be zero (you commonly don't).
> 
> The function predates our (rather late) realization that returning a
> recognizable error value in addition to setting an error leads to more
> readable code.  Today, we'd perhaps do it the way you describe below.
> 
> >> It's a bit misleading to mention this in the header though. I
> >> understand that the function should return something, which
> >> should have a some explicitly assigned value to avoid compilers
> >> or static analyzers to yell, but the written contract should be
> >> that the value is _undefined_ IMHO.
> >
> > Hrm... I think the description could be clearer, but returning 0
> > explicitly on the failure case has some benefits too.  If 0 is a
> > reasonable default for when the property isn't present (which is a
> > plausibly common case) then it means you can just get a value and
> > ignore errors.
> 
> Matter of taste.
> 
> There's no shortage of _undefined_ in C...
> 

Yeah and each compiler has its take as how to handle that.

FWIW see section 3.1 of this bachelor thesis on the topic:

https://www.cs.ru.nl/bachelors-theses/2017/Matthias_Vogelaar___4372913___Defining_the_Undefined_in_C.pdf

> >> In its present form, the only way to know if the property is
> >> valid is to pass a non-NULL errp actually. I'd rather even see
> >> that in the contract, and an assert() in the code.
> >
> > Maybe... see above.
> 
> If you think the contract could be improved, please post a patch.
> 

The contract of object_property_get_enum() which is the next function
in object.h explicitly says that the result is undefined, even if
the implementation returns 0. So I was thinking of doing the same
for object_property_get_uint().

> What assertion do you have in mind?  If it's adding assert(errp) to
> object_property_get_uint(), I'll object.  Functions should not place
> additional restrictions on @errp arguments without a compelling reason.
> 

I had such an assertion in mind but if you think this restriction is
abusive, I take your word :)

> >> An alternative would be to convert it to have a bool return
> >> value and get the actual uint result with a pointer argument.
> >
> > I don't think this is a good idea.  Returning success/failure as the
> > return value is a good rule of thumb because it reduces the amount of
> > checking of out-of-band information you need to do.  If you move to
> > returning the actual value you're trying to get out of band in this
> > sense, it kind of defeats that purpose.
> >
> > I think this one is a case where it is reasonable to make it required
> > to explicitly check the error value.
> 
> If almost all calls assign the value to a variable, like
> 
>     val = object_property_get_uint(obj, name, &err);
>     if (err) {
>         error_propagate(errp, err)
>         ... bail out ...
>     }
>     ... use val ...
> 
> then the alternative Greg proposed is easier on the eyes:
> 
>     if (!object_property_get_uint(obj, name, &val, errp)) {
>         ... bail out ...
>     }
>     ... use val ...
> 

That's what I had in mind.

> It isn't for calls that use the value without storing it in a variable
> first.
> 

$ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
60

Manual inspecting the output of

$ git grep -W object_property_get_uint -- :^{include,qom/object.c}
...

seems to be showing that most users simply ignore errors (ie. pass NULL
as 3rd argument). Then some users pass &error_abort and only a few
pass a &err or errp.

Assuming that users know what they're doing, I guess my proposal
wouldn't bring much to the code base in the end... I'm not even
sure now that it's worth changing the contract.

Cheers,

--
Greg

> >> > > Also call ERRP_GUARD() to be able to check the status of void
> >> > > function pc_dimm_plug() with *errp.
> >> 
> >> I'm now hesitating to either check *errp for object_property_get_uint()
> >> too or simply drop this patch...
> 

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Thu, 17 Sep 2020 09:38:49 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Tue, Sep 15, 2020 at 01:43:40PM +0200, Greg Kurz wrote:
>> >> On Tue, 15 Sep 2020 13:58:53 +0300
>> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> >> 
>> >> > 14.09.2020 15:35, Greg Kurz wrote:
>> >> > > As recommended in "qapi/error.h", add a bool return value to
>> >> > > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
>> >> > > of local_err in spapr_memory_plug().
>> >> > > 
>> >> > > Since object_property_get_uint() only returns 0 on failure, use
>> >> > > that as well.
>> >> > 
>> >> > Why are you sure? Can't the property be 0 itself?
>> >> > 
>> >> 
>> >> Hmmm... I've based this assumption on the header:
>> >> 
>> >>  * Returns: the value of the property, converted to an unsigned integer, or 0
>> >>  * an error occurs (including when the property value is not an integer).
>> >> 
>> >> but looking at the implementation, I don't see any check that
>> >> a property cannot be 0 indeed...
>> >
>> > Yeah, indeed I'm pretty sure it can.
>> 
>> Correct.
>> 
>> Corollary: you can't use to return value to check for failure, except
>> when you know the property cannot be zero (you commonly don't).
>> 
>> The function predates our (rather late) realization that returning a
>> recognizable error value in addition to setting an error leads to more
>> readable code.  Today, we'd perhaps do it the way you describe below.
>> 
>> >> It's a bit misleading to mention this in the header though. I
>> >> understand that the function should return something, which
>> >> should have a some explicitly assigned value to avoid compilers
>> >> or static analyzers to yell, but the written contract should be
>> >> that the value is _undefined_ IMHO.
>> >
>> > Hrm... I think the description could be clearer, but returning 0
>> > explicitly on the failure case has some benefits too.  If 0 is a
>> > reasonable default for when the property isn't present (which is a
>> > plausibly common case) then it means you can just get a value and
>> > ignore errors.
>> 
>> Matter of taste.
>> 
>> There's no shortage of _undefined_ in C...
>> 
>
> Yeah and each compiler has its take as how to handle that.
>
> FWIW see section 3.1 of this bachelor thesis on the topic:
>
> https://www.cs.ru.nl/bachelors-theses/2017/Matthias_Vogelaar___4372913___Defining_the_Undefined_in_C.pdf
>
>> >> In its present form, the only way to know if the property is
>> >> valid is to pass a non-NULL errp actually. I'd rather even see
>> >> that in the contract, and an assert() in the code.
>> >
>> > Maybe... see above.
>> 
>> If you think the contract could be improved, please post a patch.
>> 
>
> The contract of object_property_get_enum() which is the next function
> in object.h explicitly says that the result is undefined, even if
> the implementation returns 0. So I was thinking of doing the same
> for object_property_get_uint().

Let's survey actual behavior of the object_property_get*():

                           return value
    function            on success  on error
    o_p_get()           true        false
    o_p_get_str()       non-null    null
    o_p_get_link()      anything    null
    o_p_get_bool()      anything    false
    o_p_get_int()       anything    -1
    o_p_get_uint()      anything    0
    o_p_get_enum()      enum value  0 or -1

object_property_get() and object_property_get_str() have a distinct
error value.  Yes, a QAPI str cannot be null.

object_property_get_enum() has *two* error values, and one of them can
also occur as success value.  This is daft.  I'll send a patch to always
return -1 on error.  Bonus: distinct error value.

object_property_get_link(), _bool(), _int(), and _uint() don't have a
distinct error value.

>> What assertion do you have in mind?  If it's adding assert(errp) to
>> object_property_get_uint(), I'll object.  Functions should not place
>> additional restrictions on @errp arguments without a compelling reason.
>> 
>
> I had such an assertion in mind but if you think this restriction is
> abusive, I take your word :)
>
>> >> An alternative would be to convert it to have a bool return
>> >> value and get the actual uint result with a pointer argument.
>> >
>> > I don't think this is a good idea.  Returning success/failure as the
>> > return value is a good rule of thumb because it reduces the amount of
>> > checking of out-of-band information you need to do.  If you move to
>> > returning the actual value you're trying to get out of band in this
>> > sense, it kind of defeats that purpose.
>> >
>> > I think this one is a case where it is reasonable to make it required
>> > to explicitly check the error value.
>> 
>> If almost all calls assign the value to a variable, like
>> 
>>     val = object_property_get_uint(obj, name, &err);
>>     if (err) {
>>         error_propagate(errp, err)
>>         ... bail out ...
>>     }
>>     ... use val ...
>> 
>> then the alternative Greg proposed is easier on the eyes:
>> 
>>     if (!object_property_get_uint(obj, name, &val, errp)) {
>>         ... bail out ...
>>     }
>>     ... use val ...
>> 
>
> That's what I had in mind.
>
>> It isn't for calls that use the value without storing it in a variable
>> first.
>> 
>
> $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
> 60
>
> Manual inspecting the output of
>
> $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
> ...
>
> seems to be showing that most users simply ignore errors (ie. pass NULL
> as 3rd argument). Then some users pass &error_abort and only a few
> pass a &err or errp.
>
> Assuming that users know what they're doing, I guess my proposal
> wouldn't bring much to the code base in the end... I'm not even
> sure now that it's worth changing the contract.

We'd change

    val = object_property_get_uint(obj, name, &error_abort);

to

    object_property_get_uint(obj, name, &val, &error_abort);

which is not an improvement.

Most of the ones passing NULL should probably pass &error_abort
instead.  Doesn't change the argument.
    
> Cheers,
>
> --
> Greg
>
>> >> > > Also call ERRP_GUARD() to be able to check the status of void
>> >> > > function pc_dimm_plug() with *errp.
>> >> 
>> >> I'm now hesitating to either check *errp for object_property_get_uint()
>> >> too or simply drop this patch...
>> 

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Daniel P. Berrangé]

On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
> Greg Kurz <groug@kaod.org> writes:
> 
> > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
> > 60
> >
> > Manual inspecting the output of
> >
> > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
> > ...
> >
> > seems to be showing that most users simply ignore errors (ie. pass NULL
> > as 3rd argument). Then some users pass &error_abort and only a few
> > pass a &err or errp.
> >
> > Assuming that users know what they're doing, I guess my proposal
> > wouldn't bring much to the code base in the end... I'm not even
> > sure now that it's worth changing the contract.
> 
> We'd change
> 
>     val = object_property_get_uint(obj, name, &error_abort);
> 
> to
> 
>     object_property_get_uint(obj, name, &val, &error_abort);
> 
> which is not an improvement.
> 
> Most of the ones passing NULL should probably pass &error_abort
> instead.  Doesn't change the argument.

I wonder if we actually need to have an Error  parameter at all for
the getters. IIUC the only valid runtime error  is probably the case
where the property has not been set, and even then, I think properties
usually have a default value that would be returned.  All the other
errors look like programmer errors.

IOW, can we remove the Error parameter and have all the o_p_get*
method impls use error_abort.

In the rare case where a caller needs to handle a property not
being set, they can use object_property_find() as a test before
invoking the getter.

Of course requires someone motivated to audit all the case that
are not using NULL or error_abort and decide whether the attempt
at passing a local errp was actually justified or not.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Greg Kurz]

On Thu, 17 Sep 2020 13:18:51 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
> > Greg Kurz <groug@kaod.org> writes:
> > 
> > > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
> > > 60
> > >
> > > Manual inspecting the output of
> > >
> > > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
> > > ...
> > >
> > > seems to be showing that most users simply ignore errors (ie. pass NULL
> > > as 3rd argument). Then some users pass &error_abort and only a few
> > > pass a &err or errp.
> > >
> > > Assuming that users know what they're doing, I guess my proposal
> > > wouldn't bring much to the code base in the end... I'm not even
> > > sure now that it's worth changing the contract.
> > 
> > We'd change
> > 
> >     val = object_property_get_uint(obj, name, &error_abort);
> > 
> > to
> > 
> >     object_property_get_uint(obj, name, &val, &error_abort);
> > 
> > which is not an improvement.
> > 
> > Most of the ones passing NULL should probably pass &error_abort
> > instead.  Doesn't change the argument.
> 
> I wonder if we actually need to have an Error  parameter at all for
> the getters. IIUC the only valid runtime error  is probably the case
> where the property has not been set, and even then, I think properties
> usually have a default value that would be returned.  All the other
> errors look like programmer errors.
> 
> IOW, can we remove the Error parameter and have all the o_p_get*
> method impls use error_abort.
> 
> In the rare case where a caller needs to handle a property not
> being set, they can use object_property_find() as a test before
> invoking the getter.
> 

I tend to agree.

> Of course requires someone motivated to audit all the case that
> are not using NULL or error_abort and decide whether the attempt
> at passing a local errp was actually justified or not.
> 

Since I've open the can of worms, I'm volunteering to do that if
we have a consensus on the fact that "the only valid runtime
error os the case the property has not been set".

Cheers,

--
Greg

> Regards,
> Daniel

Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
>> Greg Kurz <groug@kaod.org> writes:
>> 
>> > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
>> > 60
>> >
>> > Manual inspecting the output of
>> >
>> > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
>> > ...
>> >
>> > seems to be showing that most users simply ignore errors (ie. pass NULL
>> > as 3rd argument). Then some users pass &error_abort and only a few
>> > pass a &err or errp.
>> >
>> > Assuming that users know what they're doing, I guess my proposal
>> > wouldn't bring much to the code base in the end... I'm not even
>> > sure now that it's worth changing the contract.
>> 
>> We'd change
>> 
>>     val = object_property_get_uint(obj, name, &error_abort);
>> 
>> to
>> 
>>     object_property_get_uint(obj, name, &val, &error_abort);
>> 
>> which is not an improvement.
>> 
>> Most of the ones passing NULL should probably pass &error_abort
>> instead.  Doesn't change the argument.
>
> I wonder if we actually need to have an Error  parameter at all for
> the getters. IIUC the only valid runtime error  is probably the case
> where the property has not been set, and even then, I think properties
> usually have a default value that would be returned.  All the other
> errors look like programmer errors.

From the notes I took when I last hacked a trail through this jungle...

Failure modes of

    object_property_get()
        @name not found
        @name permission, i.e. no ->get
        ->get() fails (how?)

    object_property_get_{str,bool,int,uint}()
        object_property_get_qobject()
            object_property_get() with qobject output visitor
                which see
        prop value qobject not a string / bool / int / uint

    object_property_get_enum()
        @name not found
        @typename doesn't match
        object_property_get() with string output visitor
            which see
        qapi_enum_parse()
            prop value not a value of enum @typename

    object_property_get_link()
        object_property_get_str()
            which see
        prop value does not resolve

I think most of these failures are obviously programming errors most of
the time.

Many callers treat failures as programming errors by passing
&error_abort.

Many callers ignore errors by passing NULL.  I believe most of them
should really pass &error_abort instead.  Fixing them is tedious,
because you have to check what would happen on error.  If the answer is
"chaos", fix to pass &error_abort.  But the answer can also be "works as
intended", or "beats me".

> IOW, can we remove the Error parameter and have all the o_p_get*
> method impls use error_abort.

If we fix the callers that should pass &error_abort to do so, we'll see
what remains, and whether we can drop the parameter.

> In the rare case where a caller needs to handle a property not
> being set, they can use object_property_find() as a test before
> invoking the getter.

I dislike "if (X() is going to fail) do something else; else X()".

I guess it could be okay for the narrow case of "property does not
exist".

> Of course requires someone motivated to audit all the case that
> are not using NULL or error_abort and decide whether the attempt
> at passing a local errp was actually justified or not.

We got one!  Thanks, Greg :)

[PATCH 15/15] spapr: Simplify error handling in spapr_memory_unplug_request()

[Greg Kurz]

Since object_property_get_uint() only returns 0 on failure, use
that instead of local_err, and get rid of the error propagation
overhead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d6b4de6a1c53..2eca9dc26668 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3609,7 +3609,6 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
-    Error *local_err = NULL;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
@@ -3625,9 +3624,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
 
     addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
-                                         &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                          errp);
+    if (!addr_start) {
         return;
     }
 
-- 
2.26.2

Re: [PATCH 00/15] spapr: Error handling fixes and cleanups (round 2)

[David Gibson]

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

On Mon, Sep 14, 2020 at 02:34:50PM +0200, Greg Kurz wrote:
> Yet another round of sanitizing the error handling in spapr. I've
> identified locations that needed fixing with the errp-guard.cocci
> coccinelle script. It turns out that a better result is achieved
> by fixing manually, especially by converting some void functions
> to indicate success/failure with a return value.

1..4 applied to ppc-for-5.2, I'll look at the rest in due course.

> 
> Greg Kurz (15):
>   spapr: Fix error leak in spapr_realize_vcpu()
>   ppc: Add a return value to ppc_set_compat() and ppc_set_compat_all()
>   ppc: Fix return value in cpu_post_load() error path
>   spapr: Simplify error handling in callers of ppc_set_compat()
>   spapr: Get rid of cas_check_pvr() error reporting
>   spapr: Simplify error handling in do_client_architecture_support()
>   spapr: Simplify error handling in spapr_vio_busdev_realize()
>   spapr: Add a return value to spapr_drc_attach()
>   spapr: Simplify error handling in prop_get_fdt()
>   spapr: Add a return value to spapr_set_vcpu_id()
>   spapr: Simplify error handling in spapr_cpu_core_realize()
>   spapr: Add a return value to spapr_nvdimm_validate()
>   spapr: Add a return value to spapr_check_pagesize()
>   spapr: Simplify error handling in spapr_memory_plug()
>   spapr: Simplify error handling in spapr_memory_unplug_request()
> 
>  include/hw/ppc/spapr.h        |  4 +-
>  include/hw/ppc/spapr_drc.h    |  2 +-
>  include/hw/ppc/spapr_nvdimm.h |  4 +-
>  target/ppc/cpu.h              |  4 +-
>  hw/ppc/spapr.c                | 76 ++++++++++++-----------------------
>  hw/ppc/spapr_caps.c           |  7 +++-
>  hw/ppc/spapr_cpu_core.c       | 24 +++++------
>  hw/ppc/spapr_drc.c            | 17 ++++----
>  hw/ppc/spapr_hcall.c          | 34 +++++++---------
>  hw/ppc/spapr_nvdimm.c         | 24 +++++------
>  hw/ppc/spapr_pci.c            |  5 +--
>  hw/ppc/spapr_vio.c            | 12 +++---
>  target/ppc/compat.c           | 26 +++++++-----
>  target/ppc/machine.c          |  9 +++--
>  14 files changed, 108 insertions(+), 140 deletions(-)
> 

-- 
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: 833 bytes --]

Re: [PATCH 00/15] spapr: Error handling fixes and cleanups (round 2)

[David Gibson]

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

On Wed, Sep 16, 2020 at 12:49:38PM +1000, David Gibson wrote:
> On Mon, Sep 14, 2020 at 02:34:50PM +0200, Greg Kurz wrote:
> > Yet another round of sanitizing the error handling in spapr. I've
> > identified locations that needed fixing with the errp-guard.cocci
> > coccinelle script. It turns out that a better result is achieved
> > by fixing manually, especially by converting some void functions
> > to indicate success/failure with a return value.
> 
> 1..4 applied to ppc-for-5.2, I'll look at the rest in due course.

5..13 now applied as well.

> 
> > 
> > Greg Kurz (15):
> >   spapr: Fix error leak in spapr_realize_vcpu()
> >   ppc: Add a return value to ppc_set_compat() and ppc_set_compat_all()
> >   ppc: Fix return value in cpu_post_load() error path
> >   spapr: Simplify error handling in callers of ppc_set_compat()
> >   spapr: Get rid of cas_check_pvr() error reporting
> >   spapr: Simplify error handling in do_client_architecture_support()
> >   spapr: Simplify error handling in spapr_vio_busdev_realize()
> >   spapr: Add a return value to spapr_drc_attach()
> >   spapr: Simplify error handling in prop_get_fdt()
> >   spapr: Add a return value to spapr_set_vcpu_id()
> >   spapr: Simplify error handling in spapr_cpu_core_realize()
> >   spapr: Add a return value to spapr_nvdimm_validate()
> >   spapr: Add a return value to spapr_check_pagesize()
> >   spapr: Simplify error handling in spapr_memory_plug()
> >   spapr: Simplify error handling in spapr_memory_unplug_request()
> > 
> >  include/hw/ppc/spapr.h        |  4 +-
> >  include/hw/ppc/spapr_drc.h    |  2 +-
> >  include/hw/ppc/spapr_nvdimm.h |  4 +-
> >  target/ppc/cpu.h              |  4 +-
> >  hw/ppc/spapr.c                | 76 ++++++++++++-----------------------
> >  hw/ppc/spapr_caps.c           |  7 +++-
> >  hw/ppc/spapr_cpu_core.c       | 24 +++++------
> >  hw/ppc/spapr_drc.c            | 17 ++++----
> >  hw/ppc/spapr_hcall.c          | 34 +++++++---------
> >  hw/ppc/spapr_nvdimm.c         | 24 +++++------
> >  hw/ppc/spapr_pci.c            |  5 +--
> >  hw/ppc/spapr_vio.c            | 12 +++---
> >  target/ppc/compat.c           | 26 +++++++-----
> >  target/ppc/machine.c          |  9 +++--
> >  14 files changed, 108 insertions(+), 140 deletions(-)
> > 
> 



-- 
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: 833 bytes --]