On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: > On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: > > __e820__range_update and e820__range_remove had a very similar > > implementation with a few lines different from each other, the lines > > that actually perform the modification over the e820_table. The > > similiraties were found in the checks for the different cases on how > > each entry intersects with the given range (if it does at all). These > > checks were very presice and error prone so it was not a good idea to > > have them in both places. > > Yay removing copy/paste code! :) Removing copy/paste is nice but diffstat of arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- 1 file changed, 283 insertions(+), 100 deletions(-) does not look nice even accounting for lots of comments :( I didn't look closely, but diffstat clues that the refactoring making things much more complex. > > > > I propose a refactor of those functions, given that I need to create a > > similar one for this patchset. > > The diff here is pretty hard (for me) to review; I'll need more time > to check it. What might make review easier (at least for me), is to > incrementally change these routines. i.e. separate patches to: > > - add the new infrastructure > - replace e820__range_remove > - replace __e820__range_update > > If that's not actually useful, no worries. I'll just stare at it a bit > more. :) -- Sincerely yours, Mike.
