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.vetr...@bugseng.com> >>> --- >>> 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
