loader.efi: Apply command-line DHCP overrides earlier
ClosedPublic
Actions

Authored by kgalazka on Thu, Sep 24, 8:48 PM.

Details

Reviewers

imp
manu

Commits

rGc4640126f116: loader.efi: Apply command-line DHCP overrides earlier

Summary

Ability to override DHCP options with command-line arguments
was affected by intoduction of initmd support. Initmd discovery
configures the network before loader arguments
were parsed, so a dhcp.root-path override was unavailable
during the first network configuration. Move parsing
arguments earlier and apply dhcp.root-path even if DHCP response
does not contain option 17. This allows providing a dynamic NFS
root e.g. by chain loading loader.efi from iPXE.

Signed-off-by: Krzysztof Galazka <krzysztof.galazka@intel.com>

Sponsored by: Intel Corporation
Assisted by: Github Copilot (GPT-5.6 Sol)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 77289
Build 74172: arc lint + arc unit

Event Timeline

kgalazka created this revision.Thu, Sep 24, 8:48 PM

Herald added subscribers: dab, bcran. · View Herald TranscriptThu, Sep 24, 8:48 PM

kgalazka requested review of this revision.Thu, Sep 24, 8:48 PM

Harbormaster completed remote builds in B77289: Diff 187629.Thu, Sep 24, 8:48 PM

kgalazka added a child revision: D59999: loader.efi: Retain standalone network configuration.Thu, Sep 24, 8:48 PM

The only thing that I worry about in moving this parsing earlier is side effects from env variables taking effect before we have the efi partition list, so things like currdev=disk3p4: stop working. Is that the case?

Address @imp comment.

I moved parse_args just before maybe_download_initmd.

I'm not sure if this is the correct way to test it,
but loader.efi started from EFI shell in a VM seems
to respects curdev provides as command-line argument.

Harbormaster completed remote builds in B77325: Diff 187674.Fri, Sep 25, 2:02 PM

In D59998#1376993, @kgalazka wrote:

Address @imp comment.

I moved parse_args just before maybe_download_initmd.

I'm not sure if this is the correct way to test it,
but loader.efi started from EFI shell in a VM seems
to respects curdev provides as command-line argument.

That's the right way to test it :) There's half a dozen different ways to get currdev correctly on the command line, and shell it the easiest and best because it is a common workaround to have a startup.nsh launch the boot loader because the firmware doesn't get something right.

This revision is now accepted and ready to land.Fri, Sep 25, 3:07 PM

I had claude look at this, and it likes it, but added there's a possible refinement that makes sense to me.

bootp.c:476-480 (if (tag == TAG_ROOTPATH) { if (getenv("dhcp.root-path")==NULL) val=cp; strlcpy(...) }) is now dead weight. setenv_() (line 458) already promotes DHCP option 17 into dhcp.root-path with overwrite=0 (preserves a pre-set override), and dev_net.c's new exit: block unconditionally re-copies that same env var into rootpath right after. Same value gets written twice from two files. Could collapse the bootp.c branch to a plain strlcpy(rootpath, (const char *)cp, sizeof(rootpath)) and let dev_net.c own the override entirely.

So consider this to be a nice to have addition.

kgalazka mentioned this in D59578: ice(4): Fix link bringup on driver load.Mon, Sep 28, 12:08 PM