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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.07.2023 12:23, Federico Serafini wrote:
> Change mechanically the parameter names and types of function
> declarations to be consistent with the ones used in the corresponding
> definitions so as to fix violations of MISRA C:2012 Rule 8.3 ("All
> declarations of an object or function shall use the same names and type
> qualifiers") and MISRA C:2012 Rule 8.2 ("Function types shall be in
> prototype form with named parameters").
> 
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>

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