| Message ID | 20200914123505.612812-11-groug@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 2) | expand |
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>
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>
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> >
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.
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; }
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(-)