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 17:03, 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.
> 
> This one is defined as
> 
> return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
> 
> so I'm not certain that the parameter can be dropped from that function, 

You're right, it can't. I didn't pay enough attention on the interaction
of both. So renaming it is here then as well.

Jan

> because the cascade effect
> would eliminate the need to have a 'boot_e820' in 'xen/arch/x86/setup.c' 
> afaict and since the comment says
> 
> /* A temporary copy of the e820 map that we can mess with during 
> bootstrap. */
> static struct e820map __initdata boot_e820;
> 
> I'm not sure it's a good idea to alter this call chain.
>