Page MenuHomeFreeBSD

bsdinstall: Split installation script not more than once
ClosedPublic

Authored by grembo on Jan 6 2024, 5:23 PM.
Tags
None
Referenced Files
F104985058: D43350.diff
Wed, Dec 11, 9:07 AM
Unknown Object (File)
Wed, Dec 4, 2:34 PM
Unknown Object (File)
Wed, Dec 4, 2:12 AM
Unknown Object (File)
Thu, Nov 21, 1:14 PM
Unknown Object (File)
Nov 8 2024, 3:13 PM
Unknown Object (File)
Nov 8 2024, 3:07 PM
Unknown Object (File)
Nov 8 2024, 1:47 PM
Unknown Object (File)
Nov 8 2024, 1:32 PM
Subscribers

Details

Summary

bsdinstall script uses split(1) to split the installation script
into a preamble - containing options - and an optional setup script
called installscript.

In case the setup script contains a script header ("shebang") after
the initial one, the installation script is split multiple times,
ignoring anything from that point in the file when creating
installscript.

Example:

RELEASE="14.0"
...

#!/bin/sh
echo "Populate rc.local"
cat >/etc/rc.local<<EOF
#!/bin/sh
echo booted | logger -s -t 'example'
EOF
echo "Shutdown"
shutdown -p now

This results in three files:

  • bsdinstall-installscript-aa: preamble
  • bsdinstall-installscript-ab: setup script only containing three lines (ending on cat >/etc/rc.local<<EOF)
  • bsdinstall-installscript-ac: remainder of the install script, which is never executed (and if it would, it would do the wrong thing).

Unfortunately, the split command doesn't support a "max files" option
(which would be a nice addition and an easy fix, think split -m2).

This patch aims to correct this by replacing split with awk (and
adding some more fixes).

Test Plan

Manual

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 55278
Build 52167: arc lint + arc unit

Event Timeline

grembo requested review of this revision.Jan 6 2024, 5:23 PM

Also fix for jail command

As I just learned, the jail command also supports script (nice!).
In this case, cleaning up scripts from previous runs is even
more important.

Tested locally using SCRIPT=myinstallscript bsdinstall jail.

Or you could avoid these kinds of hacks and do something like:

awk 'BEGIN {pathb=ARGV[2]; ARGV[2]=""} /^#!/{b=1} {if (b) print >pathb; else print}' "$SCRIPT" >"$TMPDIR"/bsdinstall-installscript-preamble "$TMPDIR"/bsdinstall-installscript-setup

?

(NB: names taken from the manpage)

Thanks for taking the time.

Or you could avoid these kinds of hacks and do something like:

awk 'BEGIN {pathb=ARGV[2]; ARGV[2]=""} /^#!/{b=1} {if (b) print >pathb; else print}' "$SCRIPT" >"$TMPDIR"/bsdinstall-installscript-preamble "$TMPDIR"/bsdinstall-installscript-setup

My goal was to make the change minimally invasive, so it's easy (and trivial) to review.

?

(NB: names taken from the manpage)

NB: I read the man page before deciding on a name and found "options" to be more useful to people just looking at the file system (and not necessarily reading the documentation to learn that "The preamble sets up the options for the installation"). Both work for me though (and anything is better than -aa/-ab).

grembo edited the summary of this revision. (Show Details)

Incorporate feedback given in review

@jrtc27 Retested bsdinstall jail after the change. Note that I didn't bother to quote TMPDIR, as it would've been odd in that codebase (compare use of other paths in bsdinstall, e.g., BSDINSTALL_CHROOT). Fixing these things should be done when refactoring/in a separate effort.

Only delete setup script (as preamble will always be overwritten)

@jrtc27 @imp Any additional remarks? Once landed (and if time permits) I thought about improving bsdinstall's use of quoting in a separate review.

This seems like a reasonable first step to getting accomplishing this.
Though the code repetition and general concept suggest that maybe this design point might be best revisited int the future.

In D43350#988235, @imp wrote:

This seems like a reasonable first step to getting accomplishing this.
Though the code repetition and general concept suggest that maybe this design point might be best revisited int the future.

Yeah, my primary goal was to fix the split-at-shebang bug, while there I discovered that it also might reuse files from previous runs (and that we gained SCRIPT support in the jail sub-command, where such reuse might be a lot more critical). Thanks to @jrtc27 the bugfix is pretty clean now, let's see how far I'll go into the rabbit hole of cleaning up in there (quoting issues seem worthwhile and thanks to shellcheck relatively easy to spot).

Thanks for your input, I'll land this soonish if I there is no additional feedback.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 9 2024, 6:18 PM
This revision was automatically updated to reflect the committed changes.

@jrtc27 @imp Thanks again for your timely feedback, much appreciated.