On 07.08.2023 13:12, Nicola Vetrini wrote: > >>> 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. > > Ok > >> >>> - 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. >> > > Ok, although your suggestion of breaking these renames/deletions in more > than one patch may not be applicable, > as 'kexec_reserve_area' calls 'reserve_e820_ram', which in turn calls > 'e820_change_range_type'. > Similarly, the call stack containing 'e820_add_range' includes other > calls to the modified functions, so > effectively it's best to drop the parameter everywhere all at once to > prevent accidental mistakes.
Well, this still allows splitting parameter removal changes from parameter renaming ones. >>> 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. > > Ok, so perhaps the best way is using the strategy above, although I'm > curious why in other places this > was not the preferred alternative (as the global may be dropped or the > callers may use a e820map other > than the global one, but here I recognize my lack of knowledge on the > internals of Xen). Other x86 maintainers may voice a different opinion. My take is that since we've now lived with the set of functions we have for quite a long time, problematic changes like ones you outline are not very likely to appear. Jan
