bsdinstall: restore the environment when restarting

[khorben_defora.org]

Summary

It is possible to restart the installation process upon errors, when installing normally through the auto script, or when installing a jail with the jail script. However, some values obtained interactively from the user or guessed by some scripts were kept in the environment when restarting the process; this made it impossible to re-run some steps as expected after the restart.

For instance, if a bad choice of mirror was made in the mirrorselect phase, restarting the installer remembered the choice made, and would never prompt for a different one again. Rebooting was then the only easy way out of this situation.

This change restores a pre-defined list of environment variables when restarting the installation process.

This is believed to solve PR #266987.

This is a broader and cleaner fix than D42183, which only solved this issue when choosing mirrors while setting up jails.
The patch is also available on GitHub at https://github.com/khorben/freebsd-src/tree/khorben/bsdinstall-restart-reset-env.

Test Plan

I have created a stand-alone copy of usr.sbin/bsdinstall, slightly modified in order to allow local testing without requiring a new installation image for every change.

$ git clone --branch khorben/bsdinstall-restart-reset-env https://github.com/khorben/bsdinstall.git
$ cd bsdinstall
$ less README.md
[...]
$ cat > test.sh << EOF
#!/bin/sh

BSDINSTALLDIR="\$PWD"
DESTDIR="\$BSDINSTALLDIR/destdir"
BSDINSTALL_DISTDIR="\$DESTDIR/usr/freebsd-dist"; export BSDINSTALL_DISTDIR
SRCDIR="/usr/src"

TMPDIR="\$(mktemp -d)"; export TMPDIR
BSDCFG_SHARE="\$SRCDIR/usr.sbin/bsdconfig/share" \
    BSDINSTALL_CHROOT="\$DESTDIR" \
    BSDINSTALL_CONFIGCURRENT="yes" \
    BSDINSTALL_SCRIPTS="\$BSDINSTALLDIR/scripts" \
    DISTRIBUTIONS="base.txz" \
    ./bsdinstall "\$@"
EOF
$ sh test.sh jail /tmp/jail
[select any item, confirm, deselect any item, confirm, repeat]

(Add, remove, or modify $DISTRIBUTIONS for exhaustive testing)

[jrtc27]

usr.sbin/bsdinstall/scripts/auto

155

Should we not preserve these normally too? Probably it's fine not to, but better to have all variables be handled uniformly (perhaps even you could just have a list of variables that need saving/restoring and loop over them rather than copy/pasting the code for each?)

usr.sbin/bsdinstall/scripts/jail

71

This will restore an unset variable to an empty one, which isn't *quite* the same, and means you need to be careful about :- vs - (and for some variables maybe empty-but-set would even be valid)

[khorben_defora.org]

usr.sbin/bsdinstall/scripts/auto

155

You are right, the user may also have forced a workaround to be applied, even without bsdinstall discovering its need.
It makes sense to list the variables indeed.

usr.sbin/bsdinstall/scripts/jail

71

Are you speaking about code like export DISTRIBUTIONS="${DISTRIBUTIONS:-base.txz kernel.txz}"?

From what I can tell, the shell should behave the same way whether a variable is empty or unset:

$ D=1; echo ${D:-2}
1
$ D=; echo ${D:-2}
2
$ unset D; echo ${D:-2}
2

This is also confirmed in the manual page:

${parameter:-word}
        Use Default Values.  If parameter is unset or null, the expansion
        of word is substituted; otherwise, the value of parameter is
        substituted.

Let me know if there's something I am missing :)

[jrtc27]

usr.sbin/bsdinstall/scripts/jail

71

In the parameter expansions shown previously, use of the colon in the
format results in a test for a parameter that is unset or null; omis-
sion of the colon results in a test for a parameter that is only unset.

That is, with FOO=, ${FOO-bar} gives "", unlike ${FOO:-bar}, which gives "bar". This doesn't matter for DISTRIBUTIONS, but because this difference does matter, a true environment_restore would *unset* variables that were not set, rather than set them to an empty value.

[khorben_defora.org]

This updated diff:

• Uses a pre-defined list of variables to save or restore the environment
• Lets the prefix be configured for the saved variables
• Differentiates between empty and unset variables when saving and restoring them
• Adds a few comments for clarity and consistency