Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove

[Mike Rapoport]

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.