Update bsdinstall/zfsboot to use newfs_msdos instead of an image file

[allanjude]

Summary

This is related to D12302 but does not conflict

[imp]

Apart from the fact that this enshrines too much knowledge of how to make things bootable, this is fine (since it already knows and my stuff isn't quite ready).

usr.sbin/bsdinstall/scripts/zfsboot

214 ↗

(On Diff #32910)

What does ESP mean?

854–869 ↗

(On Diff #32910)

This should be spelled something more like

f_eval_catch $funcname installboot <mumble>

here and for all the places we deal with boot blocks. There's too many places that have way too much knowledge of how to make things bootable. This script knows all kinds of crazy details. nanobsd knows. crochet knows. release/Makefile knows. It's just nuts.

I should have installboot up for review this week.

[emaste]

usr.sbin/bsdinstall/scripts/zfsboot

214 ↗

(On Diff #32910)

EFI System Partition

1589 ↗

(On Diff #32910)

quotes here are superfluous

1598–1599 ↗

(On Diff #32910)

this seems a bit strange - I'd expect an amd64 case, and maybe a default that prints an error?

[dteske]

Add comments and request changes

usr.sbin/bsdinstall/scripts/zfsboot

854–869 ↗

(On Diff #32910)

installboot sounds like analogous to grub2-* utility suite on Linux for abstracting away details. Good or Bad, I have no opinion, just pointing out that Linux has a centralized toolchain for munging boot stuffs. Would be kind of cool to have a centralized toolchain that is specific to our loader.

863–866 ↗

(On Diff #32910)

Remove unnecessary curlies around BSDINSTALL_TMPETC and ZFSBOOT_ESP_NAME variables

1587 ↗

(On Diff #32910)

Remove extraneous new/empty-line

1588 ↗

(On Diff #32910)

You should use this instead:

case "${UNAME_m:-$( uname -m )" in

which will prevent unnecessary execution of uname(1) in the event that UNAME_m is set (uname(1) will simply return UNAME_m if/when set, meaning the fork-exec to uname(1) is really a waste of time/energy/resources when UNAME_m is set).

1589 ↗

(On Diff #32910)

When in the argument-space of a command (regardless of whether it is a built-in, externally tracked executable, keyword, or special operator), the double-quotes prevent re-interpolation of the sub-shell response into the argv elements for the given command and thus the double-quotes are not "superfluous" as you may suggest. For example, should the execution of the following return nothing on stdout and an error on stderr, interpolation would result in case in if the code was simply case $( uname -m ) in instead of the situation where the sub-shell itself is quoted.

1589–1599 ↗

(On Diff #32910)

Please condense the stanzas to one line each, remove the unnecessary quotes around the patterns, move the fallback condition inside the case, and eliminate unnecessary underride duplication (if the variable has a value, no need to do the case; when NULL or unset, use case to determine value). Example given:

[ "$ZFSBOOT_ESP_NAME" ] || case "${UNAME_m:-$( uname -m )}" in
arm64) ZFSBOOT_ESP_NAME=BOOTaa64.efi ;;
  arm) ZFSBOOT_ESP_NAME=BOOTarm.efi ;;
 i386) ZFSBOOT_ESP_NAME=BOOTia32.efi ;;
    *) ZFSBOOT_ESP_NAME=BOOTx64.efi
esac

[emaste]

usr.sbin/bsdinstall/scripts/zfsboot

1588 ↗

(On Diff #32910)

I disagree -- for code that has no performance requirement, the effort required for a reader to understand the more complex construction is not worth the minuscule waste of computing resources. Consider that in nearly all cases bsdinstall is invoked from FreeBSD install media, and will not have UNAME_m set.

1589 ↗

(On Diff #32910)

Note the line this comment is on - the superfluous quotes are those on the cases -- arm64, arm, etc.

1589–1599 ↗

(On Diff #32910)

BOOTx64.efi is not a generic choice that should be an else case

[dteske]

usr.sbin/bsdinstall/scripts/zfsboot

1588 ↗

(On Diff #32910)

So you clearly do not install from CDROM wherein a first-time access is costly. Code should never be simplified at the cost of pedantic correctness. Also, your argument flies in the face of consistency within the code (I disagree with simplification, but if you're going to beat that horse, you have to be consistent intnerally within the tool's code and arguably bsdinstall and bsdconfig are both pedantically safe/correct.

1589 ↗

(On Diff #32910)

Thanks for clarification

1589–1599 ↗

(On Diff #32910)

I agree with this. As stated earlier (disregarding the syntactical changes), emaste is correct that there should be some error condition in the else case. In other words, make amd64 a stanza in the case-statement and make the fallback * case produce an error when appropriate (e.g., when ESP is required but uname gives us a machine architecture that is unsupported by ESP)

[emaste]

usr.sbin/bsdinstall/scripts/zfsboot

1588 ↗

(On Diff #32910)

How often are users installing from a CDROM and overriding UNAME_m?

And I don't see what you mean by "pedantic correctness" -- the simpler case is no less correct.

[dteske]

usr.sbin/bsdinstall/scripts/zfsboot

1588 ↗

(On Diff #32910)

More often than you would like to hear. For example, the Vicor Business Unit of FIS Global, Inc. installs from CDROM several hundreds of times per year using an external USB CDROM drive. This is a requirement due to FFIEC regulations that prohibit the use of USB external hard/thumb/flash drives.

If simpler is what you want, the new suggestion is as follows:

case "$UNAME_M" in

Line 33 of the same file:

. $BSDCFG_SHARE/common.subr || exit 1

Line 63 of /usr/share/bsdconfig/share/common.subr:

export UNAME_M="$( uname -m )" # Machine platform (i.e. i386)

This would leverage already-stored information.

To answer your question about the meaning of "pedantic correctness": true to the code's intent as defined by the original authors (Allan Jude and myself). I wrote this code and continue to write my code to be performant.

An example of targeting performance in scripts that implement a UI you need to make sure that you don't make the following assumptions:

1. That calling an externally tracked binary is without penalty
2. That the externally tracked binary is cached
3. That a filesystem event will not occur when invoking an externally tracked executable

And above all else, externally tracked executables should be minimized in loops.

The case of a CDROM and also the case of a slow and/or wonky NFS connection would be excellent examples of where calling "uname" to get information that was already obtained at the start of bsdinstall invocation unnecessarily causes delays in responsiveness.

It is often tempting to think "my code doesn't run in a loop, this call to an externally tracked executable is harmless as it is just this one script doing this once." The reality in a structured heirarchical system of menus is that your menu -- even if it exists as a stand-alone application -- may be called in a loop if another menu ties to it and the user doesn't follow what the developer thinks to be the traditional flow (and why should they be prevented from, for example, navigating into and out-of a menu many times before they decide to do something different).

Nothing teaches you better coding practice and dilligence in re-using already-available information like trying to do installs via CDROM (wherein the CDROM will spin-down during lengthy UI sessions that operate off of a single invocation of dialog; e.g., --mixed-form invocations) or slow NFS (wherein for example expanding a data center requires installing a remote datacenter by first initizling the local PXE server via some other remote PXE server with a high latency and/or low bandwidth issue given the long-distance).

[allanjude]

Updated to use loader.efi and create a startup.nsh

Still need to address some of the other feedback

Warner: IIRC we can't put the files in the freebsd/ directory in EFI until we set some EFI Vars, is that a thing yet?

[allanjude]

usr.sbin/bsdinstall/scripts/zfsboot

857 ↗

(On Diff #47076)

I changed this to 'EFISYS' to match https://svnweb.freebsd.org/base?view=revision&revision=332561

854–869 ↗

(On Diff #32910)

I agree we should switch to installboot in the future.

[allanjude]

Address the rest of the feedback from emaste and dteske

[dteske]

Additional comments following an update

usr.sbin/bsdinstall/scripts/zfsboot

860–861 ↗

(On Diff #47099)

Unparameterized, unquoted use of f_eval_catch.

Should add toward top:

CP='cp = "%s" "%s"'

And then do:

f_eval_catch $funcname cp "$CP" /boot/loader.efi "$BSDINSTALL_TMPETC/esp/efi/boot/$ZFSBOOT_ESP_NAME"

863–864 ↗

(On Diff #47099)

Curlies around ZFSBOOT_ESP_NAME and BSDINSTALL_TMPETC.

Also, just curious, can either ZFSBOOT_ESP_NAME or BSDINSTALL_TMPETC.

Also, this command is malformed. It should be something like this:

:> "$BSDINSTALL_TMPETC/esp/efi/boot/startup.nsh
f_eval_catch $funcname echo "$ECHO_APPEND" "$ZFSBOOT_ESP_NAME" $BSDINSTALL_TMPETC/esp/efi/boot/startup.sh

Or if you want, dup the ECHO_APPEND definition on line 190 of the file to:

ECHO_TO='echo "%s" > "%s"'

And then use

f_eval_catch $funcname echo "$ECHO_TO" "$ZFSBOOT_ESP_NAME" $BSDINSTALL_TMPETC/esp/efi/boot/startup.sh

1595 ↗

(On Diff #47099)

Remove empty line separating comment from its associated block to which it is attributed

863–866 ↗

(On Diff #32910)

Don't forget these curlies

[allanjude]

Address the last of dteske's feedback

Fix some 80 cols issues

[dteske]

usr.sbin/bsdinstall/scripts/zfsboot

1607 ↗

(On Diff #47143)

*)
        f_dprintf ...
        f_die
esac

[allanjude]

Last change requested by dteske