Re: [PATCH 01/13] boot: Split out the first part of fit_image_load()

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On Sat, Apr 11, 2026 at 07:03:35AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 8 Apr 2026 at 10:25, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 25, 2026 at 10:54:10AM -0600, Simon Glass wrote:
> >
> > > This function is over 250 lines long. Split out the image-selection
> > > part into a new select_image() function which handles format checking,
> > > configuration selection and image-node lookup.
> > >
> > > Take care to only call fit_get_name() when a valid node is found, since
> > > libfdt does not handle negative offsets gracefully.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > Function length isn't a problem in and of itself, especially when we
> > aren't pulling out some 4 or 5 level deep nested logic to improve
> > readability.
> 
> In my view the current code is very hard to maintain and change. I
> know this because I have tried to do it:
> 
> 1. By my count, fit_image_load() declares 20 locals. Even without deep
> nesting, tracking which variables are live at which point in a
> function of ~270 lines is extremely hard (see [1] for someone else
> that had this problem). Just to take one example, pulling
> select_image() out makes it explicit that cfg_noffset (for example) is
> only relevant during image selection and doesn't leak into the
> load/decompress path.
> 
> 2. The function has distinct phases: format checking, configuration
> selection, hash verification, and image-node lookup. So we have 'find
> what to load' vs. 'load it'. These  are much easier to understand with
> clean function boundaries, rather than an arbitrary split at a line
> count.
> 
> 3. Reviewability - when someone adds a feature to the loading path,
> they currently have to read past 90 lines of selection logic to find
> where their change fits. Smaller functions with clear contracts make
> it easier to see where new code belongs.

Or, splitting things up makes it harder to review because now what
you're reading through is over *there* instead of the next line. I find
myself having to "un-inline" function calls sometimes because we've
thrown around so many abstractions. So I stand by what I said in the
cover letter.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]