Page MenuHomeFreeBSD

nextboot: Write nextboot.conf safely
ClosedPublic

Authored by markj on Mar 31 2024, 2:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 25, 6:14 AM
Unknown Object (File)
May 14 2024, 4:45 PM
Unknown Object (File)
Apr 3 2024, 6:10 PM
Unknown Object (File)
Apr 1 2024, 6:39 PM
Subscribers

Details

Summary

As in the old nextboot.sh script:

  • First write everything to a tempfile instead of /boot/nextboot.conf.
  • fsync() the tempfile before renaming it to nextboot.conf.

While here fix handling of -f in getopt().

Fixes: fd6d47375a78 ("rescue,nextboot: Install nextboot as a link to reboot, rm nextboot.sh")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Mar 31 2024, 2:54 PM
markj created this revision.
imp requested changes to this revision.Mar 31 2024, 3:19 PM

I don’t think the raw err calls are desirable for the force case.

sbin/reboot/reboot.c
153

This is a change in behavior (here and elsewhere)... any errors in the old script in crwating nextboot mean we'd not fail to reboot anyway...

It may be more correct, but now will result in different behavior for, say, readonly /. I was torn on it, but didn't want to break automated scripting that depends on it since we'd fail with the system still running when we were forcing a reboot even if we couldn't write nextboot...

This revision now requires changes to proceed.Mar 31 2024, 3:19 PM
sbin/reboot/reboot.c
153

Ah, I forgot that this code is used not just by nextboot(8).

But, -f is already broken: neither the reboot(8) nor the nextboot(8) getopt strings include 'f', so force is always false. The documentation for -f in both nextboot.8 and reboot.8 also says that it's only used to force reboot to succeed if the kernel doesn't exist, not for other reasons.

  • Restore use of E().
  • Make nextboot/reboot handle -f.

Ok. We should put this in the relnotes then. Uggg I made a mess of things trying to sinplify :(

This revision is now accepted and ready to land.Mar 31 2024, 3:59 PM
In D44572#1016397, @imp wrote:

Ok. We should put this in the relnotes then. Uggg I made a mess of things trying to sinplify :(

Put what in the relnotes exactly? I think the simplification was fine, we just have some regressions, none of which are in 14.0.

This revision was automatically updated to reflect the committed changes.