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

[Simon Glass]

Hi Tom,

On Mon, 13 Apr 2026 at 14:18, Tom Rini <trini at konsulko.com> wrote:
>
> 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 at 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 at 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.

So...NAK for this series?

My motivation here is partly to implement new features in FIT, which
is quite painful with the current state of fit_image_load()

Regards,
Simon