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