Page MenuHomeFreeBSD

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

Authored by allanjude on Sep 11 2017, 12:28 AM.

Details

Summary

This is related to D12302 but does not conflict

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude created this revision.Sep 11 2017, 12:28 AM
imp added a comment.Sep 11 2017, 4:06 AM

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 added inline comments.Oct 13 2017, 8:38 PM
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 requested changes to this revision.Oct 13 2017, 9:34 PM

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
This revision now requires changes to proceed.Oct 13 2017, 9:34 PM
emaste added inline comments.Oct 13 2017, 9:54 PM
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 added inline comments.Oct 13 2017, 10:17 PM
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 added inline comments.Oct 14 2017, 2:27 AM
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 added inline comments.Oct 14 2017, 11:55 PM
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 this revision to Diff 47076.Aug 22 2018, 4:08 AM

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 marked 15 inline comments as done.Aug 22 2018, 2:31 PM
allanjude added inline comments.
usr.sbin/bsdinstall/scripts/zfsboot
854–869 ↗(On Diff #32910)

I agree we should switch to installboot in the future.

857 ↗(On Diff #47076)
allanjude updated this revision to Diff 47099.Aug 22 2018, 2:35 PM

Address the rest of the feedback from emaste and dteske

Additional comments following an update

usr.sbin/bsdinstall/scripts/zfsboot
863–866 ↗(On Diff #32910)

Don't forget these curlies

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

allanjude updated this revision to Diff 47143.Aug 22 2018, 11:54 PM
allanjude marked 2 inline comments as done.

Address the last of dteske's feedback

Fix some 80 cols issues

dteske added inline comments.Aug 23 2018, 10:42 PM
usr.sbin/bsdinstall/scripts/zfsboot
1607 ↗(On Diff #47143)
*)
        f_dprintf ...
        f_die
esac
allanjude updated this revision to Diff 47212.Aug 23 2018, 10:42 PM

Last change requested by dteske

dteske accepted this revision.Aug 23 2018, 10:45 PM
This revision is now accepted and ready to land.Aug 23 2018, 10:45 PM
This revision was automatically updated to reflect the committed changes.