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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 8 Aug 2023, Jan Beulich wrote:
> On 08.08.2023 09:08, Nicola Vetrini wrote:
> > On 07/08/2023 11:10, Jan Beulich wrote:
> >> 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,
> > 
> > I see another downside with this approach (I should have spotted this 
> > sooner):
> > Since e820_add_range and the other functions expected e820 as a pointer, 
> > they are
> > written like this:
> > 
> > for ( i = 0; i < e820->nr_map; ++i )
> >      {
> >          uint64_t rs = e820->map[i].addr;
> >          uint64_t re = rs + e820->map[i].size;
> > 
> >          if ( rs == e && e820->map[i].type == type )
> >          {
> >              e820->map[i].addr = s;
> >              return 1;
> >          }
> > ...
> > 
> > Dropping the parameter would either mean
> > 1. Use a local parameter that stores the address of e820, which kind of
> >     nullifies the purpose of dropping the parameter imho;
> 
> This isn't an unusual thing to do; it is only the name being short which
> may make it look "unnecessary" here. But especially if the local variable
> was made of type struct e820entry * (and updated in the for()) I think
> this could be useful overall.
> 
> > 2. Rewrite it so that it operates on a "struct e820map", which would 
> > mean
> >     substantial churn;
> > 3. Make the global a pointer, which is reminiscent of (1)
> > 
> > All in all, I do like the global renaming approach more, because it 
> > lessens
> > the amount of code that needs to change to accomodate a case of 
> > shadowing.
> 
> Well, to go that route you need to come up with a suitable new name (no
> prior proposal was really meeting that requirement) and you'd need to
> convince at least one of the x86 maintainers.

Would you be OK with your previous suggestion:

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

So:

e820_raw -> unchanged
boot_e820 -> e820_boot
e820 -> e820_table (or e820_list or e820_ranges)?