Re: [XEN PATCH v2] x86: make function declarations consistent with definitions

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 04/07/23 16:51, Jan Beulich wrote:
> On top of my earlier remark (when this was part of a series):
>
> > --- a/xen/arch/x86/cpu/mcheck/x86_mca.h
> > +++ b/xen/arch/x86/cpu/mcheck/x86_mca.h
> > @@ -113,7 +113,7 @@ static inline int mcabanks_test(int bit, struct mca_banks* 
> > banks)
> >       return test_bit(bit, banks->bank_map);
> >   }
> >   
> > -struct mca_banks *mcabanks_alloc(unsigned int nr);
> > +struct mca_banks *mcabanks_alloc(unsigned int nr_mce_banks);
> I'm not convinced here.
>
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -59,7 +59,7 @@ enum rtc_mode {
> >   static void rtc_copy_date(RTCState *s);
> >   static void rtc_set_time(RTCState *s);
> >   static inline int from_bcd(RTCState *s, int a);
> > -static inline int convert_hour(RTCState *s, int hour);
> > +static inline int convert_hour(RTCState *s, int raw);
> Nor here.
>
> > --- a/xen/arch/x86/include/asm/guest_pt.h
> > +++ b/xen/arch/x86/include/asm/guest_pt.h
> > @@ -422,7 +422,7 @@ static inline unsigned int guest_walk_to_page_order(const 
> > walk_t *gw)
> >   
> >   bool
> >   guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
> > -                  unsigned long va, walk_t *gw, uint32_t pfec,
> > +                  unsigned long va, walk_t *gw, uint32_t walk,
> >                     gfn_t top_gfn, mfn_t top_mfn, void *top_map);
> While the definition's use of "walk" makes clear why the variable is
> named the way it is despite being used with PFEC_* constants, not
> naming it "pfec" here will add confusion, as the connection to those
> constants will be lost. One will then be forced to go look at the
> definition, when looking at the declaration ought to be sufficient.
>
> I'm not going to look further, but instead repeat my suggestion to
> split this patch. Besides reducing the Cc list(s), that'll also
> help getting in parts which are uncontroversial (like e.g. the
> change to xen/arch/x86/hvm/vioapic.c).
>
> Jan
Hello Jan.

In the three cases above,
do you think changing the definitions to match the declarations
might be a solution?

Regards
--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)