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

[David Gibson]

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

signature.asc
Description: PGP signature