Re: [PATCH 01/17] fs: boot: Update fs_read_alloc() to use abuf

[Tom Rini <trini@konsulko.com>]

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

On Wed, Apr 02, 2025 at 04:46:23AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 1 Apr 2025 at 06:42, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 03:37:55PM +0100, Simon Glass wrote:
> >
> > > Using an abuf for this function simplifies returning the size and also
> > > makes it easier to free memory afterwards. Update the API and callers.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  boot/bootmeth-uclass.c | 19 ++++++++++---------
> > >  fs/fs.c                | 25 +++++++++++--------------
> > >  include/fs.h           |  8 +++++---
> > >  3 files changed, 26 insertions(+), 26 deletions(-)
> >
> > So we grow platforms by ~200 bytes:
> >             sama7g54_curiosity_nandflash: all +204 text +204
> >                u-boot: add: 6/0, grow: 2/0 bytes: 204/0 (204)
> >                  function                                   old     new   delta
> >                  abuf_realloc                                 -      76     +76
> >                  abuf_uninit_move                             -      42     +42
> >                  memdup                                       -      28     +28
> >                  abuf_uninit                                  -      24     +24
> >                  fs_read_alloc                               96     106     +10
> >                  fs_load_alloc                              114     124     +10
> >                  abuf_init                                    -      10     +10
> >                  abuf_addr                                    -       4      +4
> >
> > To move away from standard buffer usage and unwinding to move to
> > something homegrown instead. I am not a fan of growing using abuf here.
> > When it was introduced in:
> >
> > commit 67bc59df05331eaac56cd0a00219d1386130aee2
> > Author: Simon Glass <sjg@chromium.org>
> > Date:   Sat Sep 25 07:03:07 2021 -0600
> >
> >     Add support for an owned buffer
> >
> > It sounded like something for some special cases. Not something to be
> > used everywhere to be different.
> 
> From what I can tell this is a one-off hit that I believe is worth
> taking at some point. However I haven't seen a code-size reduction
> yet, so I can understand your reluctance.

I'm not sure why it's worth taking. I'm not sure that abuf is worth
pushing in to every place we allocate a buffer for strings. If someone
wants to put in effort to make our handling of strings better/safer/more
secure we should look to all of the work Kees Cook (et al) did in the
kernel and borrow that, not invent something new and U-Boot specific.

-- 
Tom

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