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, Stefano Stabellini wrote:
> 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)?

Nevermind, I saw now the updated patch