Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.08.2023 13:12, Nicola Vetrini wrote:
> 
>>> Besides the one you listed, there are these other occurrences:
>>> - xen/arch/x86/mm.c:4678 in 'arch_memory_op' as local variable 'struct
>>> e820entry'
>>
>> This probably wants renaming; my suggestion would be just "e" here.
> 
> Ok
> 
>>
>>> - xen/arch/x86/include/asm/guest/hypervisor.h:55 in
>>> 'hypervisor_e820_fixup'
>>> - xen/arch/x86/include/asm/pv/shim.h:88 in 'pv_shim_fixup'
>>
>> These can likely again have their parameters dropped, for it only
>> ever being the "e820" global which is passed. (Really I think in such
>> cases the names being the same should be permitted.)
>>
>>> - xen/arch/x86/setup.c:689 in 'kexec_reserve_area'
>>
>> This surely can quite sensibly have boot_e820 use moved into the
>> function itself.
>>
> 
> Ok, although your suggestion of breaking these renames/deletions in more 
> than one patch may not be applicable,
> as 'kexec_reserve_area' calls 'reserve_e820_ram', which in turn calls 
> 'e820_change_range_type'.
> Similarly, the call stack containing 'e820_add_range' includes other 
> calls to the modified functions, so
> effectively it's best to drop the parameter everywhere all at once to 
> prevent accidental mistakes.

Well, this still allows splitting parameter removal changes from
parameter renaming ones.

>>> We can take the first approach you suggested (which was my original
>>> attempt, but then upon feedback on other
>>> patches I reworked this patch before submitting). My doubt about it 
>>> was
>>> that it would introduce a naming
>>> inconsistency with other e820-related objects/types. Anyway, if 
>>> e820_map
>>> is not a good name, could e820_arr be it?
>>
>> But how does "arr" describe the purpose? I would have suggested a name,
>> but none I can think of (e820_real, e820_final) I'd be really happy 
>> with.
>> Just e820 is pretty likely the best name we can have here.
> 
> Ok, so perhaps the best way is using the strategy above, although I'm 
> curious why in other places this
> was not the preferred alternative (as the global may be dropped or the 
> callers may use a e820map other
> than the global one, but here I recognize my lack of knowledge on the 
> internals of Xen).

Other x86 maintainers may voice a different opinion. My take is that
since we've now lived with the set of functions we have for quite a
long time, problematic changes like ones you outline are not very
likely to appear.

Jan