Created attachment 244122 [details] Patch to revert erroneous changes in 1f7746d81f53447ac15cc99395bb714d4dd0a4da. In commit 1f7746d81f53447ac15cc99395bb714d4dd0a4da, a change was introduced that breaks scripted installs. Given an installerconfig file like this: ``` export PARTITIONS=DEFAULT export DISTRIBUTIONS="kernel.txz base.txz" export HOSTNAME=freebsd export BSDINSTALL_DISTSITE="https://download.freebsd.org/releases/amd64/13.2-RELEASE" export nonInteractive="YES" dhclient $INTERFACES #!/bin/sh sysrc ifconfig_DEFAULT=DHCP sysrc sshd_enable=YES ``` The installation will always fail with a checksum error. This happens because the remote dists are fetched like this: ``` ( exec 3>&1 export BSDINSTALL_DISTDIR=$(`dirname $0`/fetchmissingdists 2>&1 1>&3) FETCH_RESULT=$? exec 3>&- return $FETCH_RESULT ) || error "Could not fetch remote distributions" ``` There is an attempt there to change the value of $BSDINSTALL_DISTDIR to the location of the fetched files, but since this logic was placed in a subshell that change is now a no-op. Additionally, this code attempts to "return" from a subshell, but "return" is only valid in functions. The attached patch reverts this change so that both "auto" and "script" use the same logic for fetching remote dists, and scripted installs operate as intended.
let's cc the author of the original patch
Have you read the commit message of 1f7746d81f53447ac15cc99395bb714d4dd0a4da? We can't just revert it.
> Have you read the commit message... Of course...and then I read the code, which is logically incorrect, syntactically incorrect, obviously never tested, and breaks a major existing piece of functionality. It should never have been accepted in the first place. The commit also looks like only a partial attempt at a solution, because it doesn't touch the identical code in the "auto" script (nor the several additional places in "auto" that perform the same fd manipulation). I think reverting the original commit is absolutely the best course of action because it restores the ability to perform fully automated installs. If the commit was solving a real problem, we can take the time to develop a more effective solution once we have un-broken this critical feature.
I think the original commit message also failed to accurately describe the problem. It says: Throughout the bsdinstall script fd 3 is used by f_dprintf (set through $TERMINAL_STDOUT_PASSTHRU). By closing file descriptor 3 here, the final f_dprintf "Installation Completed ... does not work anymore. But what it means is: By closing file descriptor 3 here, the install will fail with "/usr/libexec/bsdinstall/script: 3: Bad file descriptor. That seems like a bigger problem than "f_dprintf...does not work anymore". I'll submit an updated patch.
Created attachment 244175 [details] Fix call to fetchmissingdists in script This applies on top of the the existing patch to correct the problem originally identified in 1f7746d81.
While you're already at it, do you intend to fix other similar pattern, you've in e.g. "auto", as well? Btw. We need this as one commit, as reverting 1f7746d81f53447ac15cc99395bb714d4dd0a4da won't fix the install as you've already noticed.
I'm was a little hesitant about touching "auto" because it seems to run without failing at the moment, but I'm happy to apply the same change there and to bundle everything into a single patch. I will post an update shortly.
It's not just "auto"; a search for "3>&-" in the bsdinstall scripts directory yields: auto:310:exec 3>&- auto:346: exec 3>&- auto:387: exec 3>&- fetchmissingdists:67: exec 3>&- hardening:54:exec 3>&- jail:72: exec 3>&- jail:109: exec 3>&- mirrorselect:93:exec 3>&- mirrorselect:122: exec 3>&- netconfig:77:exec 3>&- netconfig:191:exec 3>&- netconfig_ipv4:81:exec 3>&- netconfig_ipv6:116:exec 3>&- services:60:exec 3>&- time:49:exec 3>&- time:61:exec 3>&- Should these *all* be updated?
Created attachment 244206 [details] Avoid conflicts with fd 3
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c0e249d32c780ee8240fe8b3b8144078a8eec41f commit c0e249d32c780ee8240fe8b3b8144078a8eec41f Author: Lars Kellogg-Stedman <lars@oddbit.com> AuthorDate: 2023-08-15 15:44:02 +0000 Commit: Corvin Köhne <corvink@FreeBSD.org> CommitDate: 2023-09-21 06:41:48 +0000 bsdinstall: avoid conflicts with fd 3 Throughout the bsdinstall script fd 3 is used by f_dprintf (set through $TERMINAL_STDOUT_PASSTHRU). In several places in the bsdinstalls scripts, we use fd 3 to juggle stdout when calling out to other tools, which can cause the installer to fail with a "Bad file descriptor" error when f_dprintf attempts to use it. This commit replaces all constructs like this: exec 3>&1 SOME_VARIABLE=$(some command 2>&1 1>&3) exec 3>&- With: exec 5>&1 SOME_VARIABLE=$(some command 2>&1 1>&5) exec 5>&- PR: 273148 Reviewed by: corvink Fixes: 1f7746d81f53447ac15cc99395bb714d4dd0a4da ("bsdinstall: stop messing with file descriptors") MFC after: 1 week usr.sbin/bsdinstall/scripts/auto | 22 +++++++++++----------- usr.sbin/bsdinstall/scripts/fetchmissingdists | 6 +++--- usr.sbin/bsdinstall/scripts/hardening | 6 +++--- usr.sbin/bsdinstall/scripts/jail | 16 ++++++++-------- usr.sbin/bsdinstall/scripts/mirrorselect | 12 ++++++------ usr.sbin/bsdinstall/scripts/netconfig | 12 ++++++------ usr.sbin/bsdinstall/scripts/netconfig_ipv4 | 6 +++--- usr.sbin/bsdinstall/scripts/netconfig_ipv6 | 6 +++--- usr.sbin/bsdinstall/scripts/script | 13 ++++++------- usr.sbin/bsdinstall/scripts/services | 6 +++--- usr.sbin/bsdinstall/scripts/time | 12 ++++++------ 11 files changed, 58 insertions(+), 59 deletions(-)