This is related to D12302 but does not conflict
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19076 Build 18707: arc lint + arc unit
Event Timeline
Comment Actions
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 | ||
|---|---|---|
| 216 | What does ESP mean? | |
| 852–875 | 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. | |
Comment Actions
Add comments and request changes
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 852–875 | 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. | |
| 861–864 | Remove unnecessary curlies around BSDINSTALL_TMPETC and ZFSBOOT_ESP_NAME variables | |
| 1589 | Remove extraneous new/empty-line | |
| 1590 | You should use this instead: case "${UNAME_m:-$( uname -m )" inwhich 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). | |
| 1591 | 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. | |
| 1591–1601 | 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 | |
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 1590 | 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. | |
| 1591 | Note the line this comment is on - the superfluous quotes are those on the cases -- arm64, arm, etc. | |
| 1591–1601 | BOOTx64.efi is not a generic choice that should be an else case | |
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 1590 | 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. | |
| 1591 | Thanks for clarification | |
| 1591–1601 | 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) | |
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 1590 | 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. | |
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 1590 | 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:
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). | |
Comment Actions
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?
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 852–875 | I agree we should switch to installboot in the future. | |
| 855 | I changed this to 'EFISYS' to match https://svnweb.freebsd.org/base?view=revision&revision=332561 | |
Comment Actions
Additional comments following an update
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 861–864 | Don't forget these curlies | |
| 863–864 | 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" | |
| 866–867 | 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 | |
| 1602 | Remove empty line separating comment from its associated block to which it is attributed | |
| usr.sbin/bsdinstall/scripts/zfsboot | ||
|---|---|---|
| 1607 | *)
f_dprintf ...
f_die
esac | |