[PATCH 0/2] Convert env export and import to use getopt()
Simon Glass
sjg at chromium.org
Thu Jun 25 17:31:55 CEST 2026
Hi Quentin,
On Thu, 25 Jun 2026 at 09:29, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> On 6/24/26 8:19 PM, Tom Rini wrote:
> > On Wed, Jun 24, 2026 at 07:35:49PM +0200, Quentin Schulz wrote:
> >> On 5/22/26 7:03 PM, Tom Rini wrote:
> >>> On Wed, May 20, 2026 at 02:12:12PM -0600, Simon Glass wrote:
> >>>
> >>>> U-Boot has had a getopt() implementation for over five years but it is
> >>>> not used much; most commands hand-roll their own argv loops to spot
> >>>> -x style flags. The env export and env import sub-commands have the
> >>>> gnarliest of these parsers in nvedit.c
> >>>>
> >>>> Each one walks every -prefixed argv element by hand, opens an inner
> >>>> loop to split grouped flags and tracks a counter to catch a repeated
> >>>> format flag.
> >>>>
> >>>> This short series converts both sub-commands to getopt(). The mutex
> >>>> check for the format flags is done after the option loop, since it is
> >>>> a per-command rule rather than an option-parsing rule, and the trailing
> >>>> positional list is read straight out of argv from gs.index onwards.
> >>>>
> >>>> There is no functional change. getopt() stops at the first non-option,
> >>>> just as the hand-rolled loops did, so options still have to appear
> >>>> before the positional arguments.
> >>>>
> >>>> Each command gains a 'select GETOPT' so the parser is linked in on
> >>>> boards that do not already enable it.
> >>>>
> >>>> For firefly-rk3399:
> >>>>
> >>>> 03: cmd: nvedit: Use getopt() in env export
> >>>> aarch64: (for 1/1 boards) all -45.0 rodata +27.0 text -72.0
> >>>> 04: cmd: nvedit: Use getopt() in env import
> >>>> aarch64: (for 1/1 boards) all -27.0 rodata -55.0 text +28.0
> >>>>
> >>>>
> >>>> Simon Glass (2):
> >>>> cmd: nvedit: Use getopt() in env export
> >>>> cmd: nvedit: Use getopt() in env import
> >>>>
> >>>> cmd/Kconfig | 2 +
> >>>> cmd/nvedit.c | 186 +++++++++++++++++++++++----------------------------
> >>>> env/common.c | 2 +-
> >>>> 3 files changed, 85 insertions(+), 105 deletions(-)
> >>>
> >>> For the record, here's a link to v2:
> >>> https://lore.kernel.org/u-boot/20260519233207.2765755-1-sjg@chromium.org/
> >>> And here's the full size change for firefly-rk3399:
> >>> Summary of 3 commits for 1 boards (1 thread, 12 jobs per thread)
> >>> 01: arm: Fix typo in linker script
> >>> aarch64: w+ firefly-rk3399
> >>> +(firefly-rk3399) Image 'simple-bin' is missing external blobs and is non-functional: atf-bl31
> >>> +(firefly-rk3399)
> >>> +(firefly-rk3399) /binman/simple-bin/fit/images/@atf-SEQ/atf-bl31 (atf-bl31):
> >>> +(firefly-rk3399) See the documentation for your board. You may need to build ARM Trusted
> >>> +(firefly-rk3399) Firmware and build with BL31=/path/to/bl31.bin
> >>> +(firefly-rk3399) Image 'simple-bin' is missing optional external blobs but is still functional: tee-os
> >>> +(firefly-rk3399) /binman/simple-bin/fit/images/@tee-SEQ/tee-os (tee-os):
> >>> +(firefly-rk3399) See the documentation for your board. You may need to build Open Portable
> >>> +(firefly-rk3399) Trusted Execution Environment (OP-TEE) and build with TEE=/path/to/tee.bin
> >>> +(firefly-rk3399) Some images are invalid
> >>> 02: cmd: nvedit: Use getopt() in env export
> >>> aarch64: (for 1/1 boards) all +883.0 rodata +199.0 text +684.0
> >>> firefly-rk3399 : all +883 rodata +199 text +684
> >>> u-boot: add: 5/0, grow: 0/-2 bytes: 1184/-500 (684)
> >>> function old new delta
> >>> __getopt - 520 +520
> >>> static.bdinfo_print_all - 324 +324
> >>> print_eth - 236 +236
> >>> print_bi_dram - 92 +92
> >>> getopt_init_state - 12 +12
> >>> do_env_export 548 476 -72
> >>> do_bdinfo 588 160 -428
> >>> 03: cmd: nvedit: Use getopt() in env import
> >>> aarch64: (for 1/1 boards) all -27.0 rodata -55.0 text +28.0
> >>> firefly-rk3399 : all -27 rodata -55 text +28
> >>> u-boot: add: 0/0, grow: 1/0 bytes: 28/0 (28)
> >>> function old new delta
> >>> do_env_import 668 696 +28
> >>>
> >>> And so this is why I'm just deferring this until someone has the time to
> >>> pick up and address the underlying problems with this potential
> >>> migration that have been raised in the previous iterations.
> >>>
> >>
> >> Chiming in because I was put in Cc...
> >>
> >> Do I understand correctly that this only impacts U-Boot proper? If so, is a
> >> size increase in proper that much of a blocker? As opposed to xPL which
> >> usually resides in SRAM, I'm assuming U-Boot proper would run in RAM where I
> >> hope an additional ~700B shouldn't be too much to ask for? The issue however
> >> could be on the storage medium where U-Boot proper is located, maybe this
> >> gets the binary above the max space allowed for U-Boot proper to be stored
> >> (e.g. on some of my boards I'm (self-imposed) limited to 2000KiB) though I'm
> >> assuming it could be less than 700B if U-Boot proper is compressed for
> >> example. But I'm also lucky enough to have storage space and (D)RAM to spare
> >> on my boards.
> >>
> >> Out of the RFC, I see the strlower() part as being separate from the rest,
> >> maybe that's something we could revisit separately? Reading the cover
> >> letter, we should even spare 60B with that alone, with the added benefit of
> >> easier maintainability?
> >
> > This was just impacting U-Boot proper. However, it showed I think two
> > sets of problems. The first of which is that there was an expectation
> > that switching the code should result in some sort of savings,
> > somewhere. And it didn't. And we do have boards where the space between
> > the end of U-Boot and the start of the environment is small and so
> > global changes that aren't just bug fixes need careful consideration
> > (and they've already turned off things that aren't needed for them such
> > as EFI loader). The second thing was that the code wasn't really easier
> > to read / follow. So I don't object to the concept but I think the
> > outcome of this series was that our getopt needs to be looked at in a
> > bit more detail, and figure out if it's really designed the way we need
> > it.
> >
>
> Understood, thanks for the additional info.
For the first point, it is true - I honestly don't see much in the way
of savings. In fact I had to work (very) hard just to keep things the
same after a conversion. Granted, this work can be used for other
commands too. As I may have mentioned elsewhere, I could imagine
changing the 'cmd' function-signature to a version which is just
passed the getopt struct. I later did some experimentation and it did
help, saving about 30 bytes (from memory) for each command.
For the second point, getopt() is easy enough to follow for regular
commands, but U-Boot does have a lot of hand-parsed pieces that
translate less well to getopt(). When I chose what to convert I
deliberately chose the odd things so that we wouldn't go ahead with
the conversion and then discover lots of pain later. Better to go in
with eyes open.
For your third point, when I see size increasing I tend to look around
randomly for things to chop. So I think it is better to keep the
strlower() win in the bag, ready for use when we need it to maintain
code size.
My vote (as I may have mentioned :-) is to bite the bullet and enable
getopt() for most boards...improving the architecture as we go. I
doubt anyone would take on converting ~1000 U-Boot commands in one go,
so we have to start somewhere.
Regards,
Simon
More information about the U-Boot
mailing list