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