On Sun, Aug 13, 2023 at 04:42:49PM -0600, Theo de Raadt wrote:
> Stuart Henderson <[email protected]> wrote:
>
> > On 2023/08/13 11:44, Andrew Hewus Fresh wrote:
> > > My laptop doesn't have the fastest wifi and sysupgrade already uses a
> > > progress bar to show what it's doing, so I'd really like to provide more
> > > feedback on what it is doing:
> >
> > Does a single -v give enough feedback?
>
> As shown below, -vv is ridiculously verbose.
>
> I think -v is also too verbose.
That makes sense to me. Here's a fairly intrusive patch that in the
normal case prints the status line "as we go" so you can see that it is
doing something. Errors from ftp are not super friendly, although I'm
not entirely sure what it should do yet, so right now I do nothing.
In the normal and single -v case, it also adds a spinner while it is
downloading if there STDOUT is a tty.
We now go in two phases for install/update where we first detect what
needs to be done and then do it so we can separate install from
download.
In any case, I think this is ready for wider testing.
Comments, suggestions, OK?
Index: fw_update.sh
===================================================================
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
retrieving revision 1.44
diff -u -p -r1.44 fw_update.sh
--- fw_update.sh 12 Dec 2022 02:30:51 -0000 1.44
+++ fw_update.sh 19 Aug 2023 21:49:39 -0000
@@ -40,18 +40,39 @@ DELETE=false
DOWNLOAD=true
INSTALL=true
LOCALSRC=
+ENABLE_SPINNER=false
+[ -t 1 ] && ENABLE_SPINNER=true
+
+integer STATUS_FD=1
+integer WARN_FD=2
+FD_DIR=
unset FTPPID
unset LOCKPID
unset FWPKGTMP
REMOVE_LOCALSRC=false
+
+status() { echo -n "$*" >&"$STATUS_FD"; }
+warn() { echo "$*" >&"$WARN_FD"; }
+
cleanup() {
set +o errexit # ignore errors from killing ftp
+
+ if [ -d "$FD_DIR" ]; then
+ echo "" >&"$STATUS_FD"
+ exec 4>&-
+
+ [ -s "$FD_DIR/status" ] && cat "$FD_DIR/status"
+ [ -s "$FD_DIR/warn" ] && cat "$FD_DIR/warn" >&2
+
+ rm -rf "$FD_DIR"
+ fi
+
[ "${FTPPID:-}" ] && kill -TERM -"$FTPPID" 2>/dev/null
[ "${LOCKPID:-}" ] && kill -TERM -"$LOCKPID" 2>/dev/null
[ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
"$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
- [ -e "${CFILE}" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
+ [ -e "$CFILE" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
}
trap cleanup EXIT
@@ -70,6 +91,22 @@ tmpdir() {
echo "$_dir"
}
+spin() {
+ if ! "$ENABLE_SPINNER"; then
+ sleep 1
+ return 0
+ fi
+
+ {
+ echo -n ' '
+ for p in '/' '-' '\\' '|'; do
+ echo -n '\010'"$p"
+ sleep 0.25
+ done
+ echo -n "\010 \010\010"
+ }>/dev/tty
+}
+
fetch() {
local _src="${FWURL}/${1##*/}" _dst=$1 _user=_file _exit _error=''
@@ -99,13 +136,13 @@ fetch() {
if [[ $_last -ne $5 ]]; then
_last=$5
SECONDS=0
- sleep 1
+ spin
else
kill -INT -"$FTPPID" 2>/dev/null
_error=" (timed out)"
fi
else
- sleep 1
+ spin
fi
done
@@ -118,7 +155,7 @@ fetch() {
if [ "$_exit" -ne 0 ]; then
rm -f "$_dst"
- echo "Cannot fetch $_src$_error" >&2
+ warn "Cannot fetch $_src$_error"
return 1
fi
@@ -133,7 +170,7 @@ check_cfile() {
[ -s "$CFILE" ] || return 1
return 0
fi
- if ! fetch_cfile "$@"; then
+ if ! fetch_cfile; then
echo -n > "$CFILE"
return 1
fi
@@ -146,10 +183,10 @@ fetch_cfile() {
fetch "$CFILE" || return 1
set -o noclobber
! signify -qVep "$FWPUB_KEY" -x "$CFILE" -m "$CFILE" &&
- echo "Signature check of SHA256.sig failed" >&2 &&
+ warn "Signature check of SHA256.sig failed" &&
rm -f "$CFILE" && return 1
elif [ ! -e "$CFILE" ]; then
- echo "${0##*/}: $CFILE: No such file or directory" >&2
+ warn "${0##*/}: $CFILE: No such file or directory"
return 1
fi
@@ -159,14 +196,25 @@ fetch_cfile() {
verify() {
check_cfile || return 1
# The installer sha256 lacks -C, do it by hand
- if ! fgrep -qx "SHA256 (${1##*/}) = $( /bin/sha256 -qb "$1" )"
"$CFILE"; then
- ((VERBOSE != 1)) && echo "Checksum test for ${1##*/} failed."
>&2
+ if ! grep -Fqx "SHA256 (${1##*/}) = $( /bin/sha256 -qb "$1" )" "$CFILE"
+ then
+ ((VERBOSE != 1)) && warn "Checksum test for ${1##*/} failed."
return 1
fi
return 0
}
+# When verifying existing files that we are going to re-download
+# if VERBOSE is 0, don't show the checksum failure of an existing file.
+verify_existing() {
+ local _v=$VERBOSE
+ check_cfile || return 1
+
+ ((_v == 0)) && "$DOWNLOAD" && _v=1
+ ( VERBOSE=$_v verify "$@" )
+}
+
firmware_in_dmesg() {
local IFS
local _d _m _dmesgtail _last='' _nl='
@@ -187,7 +235,7 @@ firmware_in_dmesg() {
case $# in
1|2|3) [[ $_dmesgtail = *$1*([!$_nl])${2-}*([!$_nl])${3-}*
]] || continue;;
- *) echo "${0##*/}: Bad pattern '${_m#$_nl}' in $FWPATTERNS"
>&2; exit 1 ;;
+ *) warn "${0##*/}: Bad pattern '${_m#$_nl}' in
$FWPATTERNS"; exit 1 ;;
esac
echo "$_d"
@@ -222,7 +270,7 @@ lock_db() {
$|=1;
lock_db(0);
-
+
say $$;
sleep;
EOL
@@ -329,7 +377,7 @@ delete_firmware() {
if [ ! -e "$_cwd/+CONTENTS" ] ||
! grep -Fxq '@option firmware' "$_cwd/+CONTENTS"; then
- echo "${0##*/}: $_pkg does not appear to be firmware" >&2
+ warn "${0##*/}: $_pkg does not appear to be firmware"
return 2
fi
@@ -389,17 +437,20 @@ do
p) LOCALSRC="$OPTARG" ;;
v) ((++VERBOSE)) ;;
:)
- echo "${0##*/}: option requires an argument -- -$OPTARG" >&2
+ warn "${0##*/}: option requires an argument -- -$OPTARG"
usage
;;
?)
- echo "${0##*/}: unknown option -- -$OPTARG" >&2
+ warn "${0##*/}: unknown option -- -$OPTARG"
usage
;;
esac
done
shift $((OPTIND - 1))
+# Progress bars, not spinner When VERBOSE > 1
+((VERBOSE > 1)) && ENABLE_SPINNER=false
+
if [ "$LOCALSRC" ]; then
if [[ $LOCALSRC = @(ftp|http?(s))://* ]]; then
FWURL="${LOCALSRC}"
@@ -407,7 +458,7 @@ if [ "$LOCALSRC" ]; then
else
LOCALSRC="${LOCALSRC#file:}"
! [ -d "$LOCALSRC" ] &&
- echo "The path must be a URL or an existing directory" >&2
&&
+ warn "The path must be a URL or an existing directory" &&
exit 1
fi
fi
@@ -424,7 +475,7 @@ if [ "$OPT_F" ]; then
rm -f "$LOCALSRC/$CFILE-OLD"
else
mv "$LOCALSRC/$CFILE-OLD" "$LOCALSRC/$CFILE"
- echo "Using existing $CFILE" >&2
+ warn "Using existing $CFILE"
fi
fi
elif [ "$LOCALSRC" ]; then
@@ -432,14 +483,34 @@ elif [ "$LOCALSRC" ]; then
fi
if [ -x /usr/bin/id ] && [ "$(/usr/bin/id -u)" != 0 ]; then
- echo "need root privileges" >&2
+ warn "need root privileges"
exit 1
fi
set -sA devices -- "$@"
+# In the normal case, we output the status line piecemeal
+# so we save warnings to output at the end to not disrupt
+# the single line status.
+# Actual errors from things like ftp will stil interrupt,
+# but it's impossible to know if it's a message people need
+# to see now or something that can wait.
+# In the verbose case, we instead print out single lines
+# or progress bars for each thing we are doing,
+# so instead we save up the final status line for the end.
+FD_DIR="$( tmpdir "${DESTDIR}/tmp/${0##*/}-fd" )"
+if ((VERBOSE)); then
+ exec 4>"${FD_DIR}/status"
+ STATUS_FD=4
+else
+ exec 4>"${FD_DIR}/warn"
+ WARN_FD=4
+fi
+
+status "${0##*/}:"
+
if "$DELETE"; then
- [ "$OPT_F" ] && echo "Cannot use -F and -d" >&2 && usage
+ [ "$OPT_F" ] && warn "Cannot use -F and -d" && usage
lock_db
# Show the "Uninstall" message when just deleting not upgrading
@@ -447,7 +518,7 @@ if "$DELETE"; then
set -A installed
if [ "${devices[*]:-}" ]; then
- "$ALL" && echo "Cannot use -a and devices/files" >&2 && usage
+ "$ALL" && warn "Cannot use -a and devices/files" && usage
set -A installed -- $(
for d in "${devices[@]}"; do
@@ -460,7 +531,7 @@ if "$DELETE"; then
if [ "${i[*]:-}" ]; then
echo "${i[@]}"
else
- echo "No firmware found for '$d'" >&2
+ warn "No firmware found for '$d'"
fi
done
)
@@ -468,20 +539,22 @@ if "$DELETE"; then
set -A installed -- $( installed_firmware '*' '-firmware-' '*' )
fi
- deleted=''
+ status " delete "
+
+ comma=''
if [ "${installed:-}" ]; then
for fw in "${installed[@]}"; do
+ status "$comma$( firmware_devicename "$fw" )"
+ comma=,
if "$DRYRUN"; then
((VERBOSE)) && echo "Delete $fw"
else
delete_firmware "$fw" || continue
fi
- deleted="$deleted,$( firmware_devicename "$fw" )"
done
fi
- deleted="${deleted#,}"
- echo "${0:##*/}: deleted ${deleted:-none}";
+ [ "$comma" ] || status none
exit
fi
@@ -494,7 +567,7 @@ fi
CFILE="$LOCALSRC/$CFILE"
if [ "${devices[*]:-}" ]; then
- "$ALL" && echo "Cannot use -a and devices/files" >&2 && usage
+ "$ALL" && warn "Cannot use -a and devices/files" && usage
else
((VERBOSE > 1)) && echo -n "Detect firmware ..."
set -sA devices -- $( detect_firmware )
@@ -503,10 +576,11 @@ else
fi
-added=''
-updated=''
+set -A add ''
+set -A update ''
kept=''
unregister=''
+
if [ "${devices[*]:-}" ]; then
lock_db
for f in "${devices[@]}"; do
@@ -519,44 +593,53 @@ if [ "${devices[*]:-}" ]; then
if "$INSTALL" && unregister_firmware "$d"; then
unregister="$unregister,$d"
else
- echo "Unable to find firmware for $d"
>&2
+ warn "Unable to find firmware for $d"
fi
continue
fi
f="$LOCALSRC/$f"
elif ! "$INSTALL" && ! grep -Fq "($f)" "$CFILE" ; then
- echo "Cannot download local file $f" >&2
+ warn "Cannot download local file $f"
exit 1
else
# Don't verify files specified on the command-line
verify_existing=false
fi
- set -A installed -- $( installed_firmware '' "$d-firmware-" '*'
)
-
- if "$INSTALL" && [ "${installed[*]:-}" ]; then
- for i in "${installed[@]}"; do
- if [ "${f##*/}" = "$i.tgz" ]; then
- ((VERBOSE > 2)) && echo "Keep $i"
- kept="$kept,$d"
- continue 2
- fi
- done
+ set -A installed
+ if "$INSTALL"; then
+ set -A installed -- \
+ $( installed_firmware '' "$d-firmware-" '*' )
+
+ if [ "${installed[*]:-}" ]; then
+ for i in "${installed[@]}"; do
+ if [ "${f##*/}" = "$i.tgz" ]; then
+ ((VERBOSE > 2)) \
+ && echo "Keep $i"
+ kept="$kept,$d"
+ continue 2
+ fi
+ done
+ fi
fi
- pending_status=false
if "$verify_existing" && [ -e "$f" ]; then
+ pending_status=false
if ((VERBOSE == 1)); then
echo -n "Verify ${f##*/} ..."
pending_status=true
elif ((VERBOSE > 1)) && ! "$INSTALL"; then
- echo "Keep/Verify ${f##*/}"
+ echo "Keep/Verify ${f##*/}"
fi
- if "$DRYRUN" || verify "$f"; then
- "$INSTALL" || kept="$kept,$d"
+ if "$DRYRUN" || verify_existing "$f"; then
+ "$pending_status" && echo " done."
+ if ! "$INSTALL"; then
+ kept="$kept,$d"
+ continue
+ fi
elif "$DOWNLOAD"; then
- ((VERBOSE == 1)) && echo " failed."
+ "$pending_status" && echo " failed."
((VERBOSE > 1)) && echo "Refetching $f"
rm -f "$f"
else
@@ -565,67 +648,91 @@ if [ "${devices[*]:-}" ]; then
fi
fi
- if [ -e "$f" ]; then
- "$pending_status" && ! "$INSTALL" && echo " done."
- elif "$DOWNLOAD"; then
- if "$DRYRUN"; then
- ((VERBOSE)) && echo "Get/Verify ${f##*/}"
- else
- if ((VERBOSE == 1)); then
- echo -n "Get/Verify ${f##*/} ..."
- pending_status=true
- fi
- fetch "$f" &&
- verify "$f" || {
- "$pending_status" && echo " failed."
- continue
- }
- "$pending_status" && ! "$INSTALL" && echo "
done."
- fi
- "$INSTALL" || added="$added,$d"
- elif "$INSTALL"; then
- echo "Cannot install ${f##*/}, not found" >&2
- continue
+ if [ "${installed[*]:-}" ]; then
+ set -A update -- "${update[@]}" "$f"
+ else
+ set -A add -- "${add[@]}" "$f"
fi
- "$INSTALL" || continue
+ done
+fi
- update="Install"
- if [ "${installed[*]:-}" ]; then
- update="Update"
- for i in "${installed[@]}"; do
- "$DRYRUN" || delete_firmware "$i"
- done
- fi
+if "$INSTALL"; then
+ status " add "
+else
+ status " download "
+fi
+action=Install
+comma=''
+[ "${add[*]}" ] || status none
+for f in "${add[@]}" _update_ "${update[@]}"; do
+ [ "$f" ] || continue
+ if [ "$f" = _update_ ]; then
+ comma=''
+ "$INSTALL" || continue
+ action=Update
+ status "; update "
+ [ "${update[*]}" ] || status none
+ continue
+ fi
+ d="$( firmware_devicename "$f" )"
+ status "$comma$d"
+ comma=,
+
+ pending_status=false
+ if [ -e "$f" ]; then
if "$DRYRUN"; then
- ((VERBOSE)) && echo "$update $f"
+ ((VERBOSE)) && echo "$action ${f##*/}"
else
- if ((VERBOSE == 1)) && ! "$pending_status"; then
+ if ((VERBOSE == 1)); then
echo -n "Install ${f##*/} ..."
pending_status=true
fi
- add_firmware "$f" "$update"
+ fi
+ elif "$DOWNLOAD"; then
+ if "$DRYRUN"; then
+ ((VERBOSE)) && echo "Get/Verify ${f##*/}"
+ else
+ if ((VERBOSE == 1)); then
+ echo -n "Get/Verify ${f##*/} ..."
+ pending_status=true
+ fi
+ fetch "$f" &&
+ verify "$f" || {
+ "$pending_status" && echo " failed."
+ continue
+ }
+ fi
+ elif "$INSTALL"; then
+ warn "Cannot install ${f##*/}, not found"
+ continue
+ fi
+
+ if ! "$INSTALL"; then
+ "$pending_status" && echo " done."
+ continue
+ fi
+
+ if ! "$DRYRUN"; then
+ if [ "$action" = Update ]; then
+ for i in $( installed_firmware '' "$d-firmware-" '*' )
+ do
+ delete_firmware "$i"
+ done
fi
- f="${f##*/}"
- f="${f%.tgz}"
- if [ "$update" = Install ]; then
- "$pending_status" && echo " installed."
- added="$added,$d"
+ add_firmware "$f" "$action"
+ fi
+
+ if "$pending_status"; then
+ if [ "$action" = Install ]; then
+ echo " installed."
else
- "$pending_status" && echo " updated."
- updated="$updated,$d"
+ echo " updated."
fi
- done
-fi
+ fi
+done
-added="${added:#,}"
-updated="${updated:#,}"
-kept="${kept:#,}"
-[ "${unregister:-}" ] && unregister="; unregistered ${unregister:#,}"
-if "$INSTALL"; then
- echo "${0##*/}: added ${added:-none}; updated ${updated:-none}; kept
${kept:-none}${unregister}"
-else
- echo "${0##*/}: downloaded ${added:-none}; kept
${kept:-none}${unregister}"
-fi
+[ "${unregister:-}" ] && status "; unregister ${unregister:#,}"
+[ "${kept}" ] && status "; keep ${kept:#,}"
