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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2023 15:43, Federico Serafini wrote:
> 
> 
> 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).
> 
> In the three cases above,
> do you think changing the definitions to match the declarations
> might be a solution?

In the first case yes. In the 2nd one I'm not sure, as the function
already has a variable named "hour". The 3rd one is probably better
left out of sync (and be deviated), but I'm open to differing views
from either or both of the other x86 maintainers.

Jan