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 10:59, Nicola Vetrini wrote:
> On 07/08/2023 10:09, Jan Beulich wrote:
>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>> The variable declared in the header file 
>>> 'xen/arch/x86/include/asm/e820.h'
>>> is shadowed by many function parameters, so it is renamed to avoid 
>>> these
>>> violations.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>> ---
>>> This patch is similar to other renames done on previous patches, and 
>>> the
>>> preferred strategy there was to rename the global variable. This one
>>> has more occurrences that are spread in various files, but
>>> the general pattern is the same.
>>
>> Still I think it would be better done the other way around, and perhaps 
>> in
>> more than a single patch. It looks like "many == 3", i.e.
>> - e820_add_range(), which is only ever called with "e820" as its 
>> argument,
>>   and hence the parameter could be dropped,
>> - e820_change_range_type(), which is in the same situation, and
>> - reserve_e820_ram(), which wants its parameter renamed.
>> Alternatively, if we really were to change the name of the global, we'd
>> want to take a more complete approach: Right now we have e820_raw[],
>> boot_e820[], and e820[]. We'd want them to follow a uniform naming 
>> scheme
>> then (e820_ first or _e820 last), with the other part of the name 
>> suitably
>> describing the purpose (which "map" doesn't do).
> 
> 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.

> - 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.

> 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.

Jan