loader: lua: test that /boot exists first
ClosedPublic
Actions

Authored by manu on Dec 13 2021, 1:24 PM.

Details

Reviewers

kevans
imp

Group Reviewers

Loader

Commits

rG28d2b8b70047: loader: lua: test that /boot exists first
rGc4dc907254a4: loader: lua: test that /boot exists first

Summary

Otherwise on fs like tftp where no directory listing is possible we fail
on the .dir method.

MFC after: 2 weeks
Sponsored by: Beckhoff Automation GmbH & Co. KG

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 43334
Build 40222: arc lint + arc unit

Event Timeline

manu created this revision.Dec 13 2021, 1:24 PM

Herald added a subscriber: dab. · View Herald TranscriptDec 13 2021, 1:24 PM

manu requested review of this revision.Dec 13 2021, 1:24 PM

kevans added inline comments.Dec 13 2021, 4:41 PM

stand/lua/core.lua
251	Any objection to moving this up above the autodetect bailout and doing something like... if not lfs.attributes("/boot", "mode") then autodetect = "no" loader.setenv("kernels_autodetect", "NO") end ? Two benefits: adds a way to detect that autodetection was skipped rather than it failed less important, avoid a second stat() on /boot if we re-enter this (e.g., drop to loader prompt and re-enter)

I'd prefer that we had a variable for "/boot" but that's orthogonal to this change.
However, we need a comment I think for why we do this.
I approved, but that's contingent on expanding the comment to explain why we short-circuit.

stand/lua/core.lua
251	I'm surprised we don't have a variable for "/boot" However, you should add a comment above explaining why we short-circuit here, drawn from the commit message. It's weird enough that I think explaining why here will likely avert churn in this area in the future.