From: Andrew Hewus Fresh <andrew@afresh1.com>
Subject: Re: Better SIGINFO for fw_update(8)
To: tech@openbsd.org
Date: Mon, 3 Nov 2025 18:09:17 -0800

On Thu, Oct 30, 2025 at 09:51:39AM -0700, Andrew Hewus Fresh wrote:
> On Tue, Oct 28, 2025 at 11:05:12AM +0100, Mark Kettenis wrote:
> > > Date: Mon, 27 Oct 2025 20:39:46 -0700
> > > From: Andrew Hewus Fresh <andrew@afresh1.com>
> > > 
> > > On Sun, Oct 26, 2025 at 03:24:57PM -0700, Andrew Hewus Fresh wrote:
> > > > Theo mentioned that sometimes fw_update takes a while and sending a
> > > > SIGINFO doesn't provide anthing useful.  Unfortunately, because when
> > > > VERBOSE < 2 we save ftp output for later it's not super straight
> > > > forward.  Saving the partially output lines so we can print them again
> > > > after the info was also a bit challenging, but it looks much nicer.
> > > > Hopefully we don't get a partially truncated INFO output in that case,
> > > > but I'm not sure how to guarantee that.
> > > > 
> > > > One issue is that I am not sure what is catching the SIGINFO and
> > > > printing "load: 0.19  cmd: ksh 97111 [running] 0.02u 0.04s 0% 190k", but
> > > > I haven't looked terribly hard.  `trap "" INFO` doesn't stop it though.
> > > > 
> > > > Right now we it only traps during "fetch", but we could add another more
> > > > global handler and variable that prints out like "Install
> > > > whatever-firmware" or "Remove whatever" if it were useful.
> > > > 
> > > > Comments, suggestions (especially on the "load:..." output), OK?
> > > 
> > > I got some feedback that the "load: ..." was coming from the kernel and
> > > `stty nokerninfo` would let me toggle it.  Here's the updated patch
> > > with, what I think, looks pretty nice, but not sure if we need other
> > > info and could use a brilliant solution for making sure that we get all
> > > the info out of _ftp_errors.  It is probably good enough, but correct
> > > would be nice.
> > 
> > So what happens if I interrupt the script with ^C?  Do I end up with
> > my terminal in the "stty nokerninfo" state?
>  
> It does need restoring in the cleanup handler.  I added something I
> didn't like there, but then got reminded of `stty -g` which had been
> nagging at me.  
> 
> > To me this all seems a lot of complexity for little gain...
> 
> It makes people happy.  That's worthwhile.

Here's another version that adds a more global SIGINFO handler.

Unfortunately, if I mash ^T and let key repeat go sometimes one of the
commands run by the shell script picks up that SIGINFO and exits causing
the installation of the firmware to fail.  I have not yet figured out
where this is happening, but will keep digging.

I welcome suggestions (including better wording for the info messages) :-)


Index: fw_update.sh
===================================================================
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
diff -u -p -r1.65 fw_update.sh
--- fw_update.sh	12 May 2025 23:48:12 -0000	1.65
+++ fw_update.sh	4 Nov 2025 00:52:09 -0000
@@ -39,6 +39,7 @@ ENABLE_SPINNER=false
 integer STATUS_FD=1
 integer WARN_FD=2
 FD_DIR=
+LAST_STATUS=
 
 unset FTPPID
 unset LOCKPID
@@ -46,11 +47,33 @@ unset FWPKGTMP
 REMOVE_LOCALSRC=false
 DROP_PRIVS=true
 
-status() { echo -n "$*" >&"$STATUS_FD"; }
-warn()   { echo    "$*" >&"$WARN_FD"; }
+INFO="Initializing"
+STTY_SETTINGS=`stty -g`
+
+status() { 
+	[ ! "$LAST_STATUS" ] || echo -n "$*" >> "$LAST_STATUS"
+	echo -n "$*" >&"$STATUS_FD"
+}
+warn() { echo "$*" >&"$WARN_FD"; }
+
+info() (
+	trap "" INFO
+	# If there was a last_status start the info on the next line
+	# and maybe overwrite the spinner.
+	[ ! -s "$LAST_STATUS" ] || echo " "
+
+	echo "$INFO" >&2
+
+	# Re-print the last_status so the line looks right.
+	[ ! -s "$LAST_STATUS" ] || cat "$LAST_STATUS"
+
+)
+trap info INFO
+stty nokerninfo
 
 cleanup() {
 	set +o errexit # ignore errors from killing ftp
+	INFO="Cleaning up"
 
 	if [ -d "$FD_DIR" ]; then
 		echo "" >&"$STATUS_FD"
@@ -68,6 +91,8 @@ cleanup() {
 	[ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
 	"$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
 	[ -e "$CFILE" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
+
+	stty "$STTY_SETTINGS"
 }
 trap cleanup EXIT
 
@@ -101,6 +126,47 @@ spin() {
 	}>/dev/tty
 }
 
+ftp_info() (
+	local _src="$1" _ftp_errors="$2"
+	local _timeout=30 # tenths of a second
+	local _signaled=true
+	trap "" INFO
+
+	# If there was a last_status start the info on the next line
+	# and maybe overwrite the spinner.
+	[ ! -s "$LAST_STATUS" ] || echo " "
+
+	echo "Fetching $_src" >&2
+
+	kill -INFO -"$FTPPID" 2>/dev/null || {
+	    #echo "Failed to signal $FTPPID";# return 0;
+	    _signaled=false
+	}
+
+	# If VERBOSE is than 2, we are storing its STDERR in _ftp_errors,
+	# and we need special handling, otherwise it outputs as normal.
+	if "$_signaled" && (( VERBOSE < 2 )); then
+        	# Normally in this case ftp is quiet until it prints INFO
+        	# but sometimes we never get any info so give up after a bit.
+		# This is a bit of a race and the full output may not be there,
+		# but I don't have a good solution for that.
+		while [ ! -s "$_ftp_errors" ]; do
+			(( _timeout-- > 0 )) || return 0
+			sleep 0.1
+		done
+
+		# Output the info and truncate
+		# so we can print additional output later
+		cat "$_ftp_errors" >&2
+		>|"$_ftp_errors"
+	fi
+
+	# Re-print the last_status so the line looks right.
+	[ ! -s "$LAST_STATUS" ] || cat "$LAST_STATUS"
+
+	return 0
+)
+
 fetch() {
 	local _src="${FWURL}/${1##*/}" _dst=$1 _user=_file _exit _error=''
 	local _ftp_errors="$FD_DIR/ftp_errors"
@@ -127,6 +193,8 @@ fetch() {
 	) & FTPPID=$!
 	set +o monitor
 
+	trap 'ftp_info "$_src" "$_ftp_errors"' INFO
+
 	SECONDS=0
 	_last=0
 	while kill -0 -"$FTPPID" 2>/dev/null; do
@@ -150,6 +218,7 @@ fetch() {
 	_exit=$?
 	set -o errexit
 
+	trap info INFO
 	unset FTPPID
 
 	if ((_exit != 0)); then
@@ -582,6 +651,7 @@ fi
 set -sA devices -- "$@"
 
 FD_DIR="$( tmpdir "${DESTDIR}/tmp/${0##*/}-fd" )"
+LAST_STATUS=$FD_DIR/last_status
 # When being verbose, save the status line for the end.
 if ((VERBOSE)); then
 	exec 3>"${FD_DIR}/status"
@@ -597,6 +667,7 @@ status "${0##*/}:"
 
 if "$DELETE"; then
 	! "$INSTALL" && warn "Cannot use -F and -d" && usage
+	INFO="Deleting firmware"
 	lock_db
 
 	# Show the "Uninstall" message when just deleting not upgrading
@@ -641,6 +712,7 @@ if "$DELETE"; then
 	comma=''
 	if [ "${installed:-}" ]; then
 		for fw in "${installed[@]}"; do
+			INFO="Removing $fw"
 			status "$comma$( firmware_devicename "$fw" )"
 			comma=,
 			if "$DRYRUN"; then
@@ -676,8 +748,10 @@ CFILE="$LOCALSRC/$CFILE"
 if [ "${devices[*]:-}" ]; then
 	"$ALL" && warn "Cannot use -a and devices/files" && usage
 elif "$ALL"; then
+	INFO="Getting available firmware"
 	set -sA devices -- $( available_firmware )
 else
+	INFO="Detecting firmware"
 	((VERBOSE > 1)) && echo -n "Detect firmware ..."
 	set -sA devices -- $( detect_firmware )
 	((VERBOSE > 1)) &&
@@ -705,6 +779,7 @@ if [ "${devices[*]:-}" ]; then
 
 		verify_existing=true
 		if [ "$f" = "$d" ]; then
+			INFO="Finding firmware for $d"
 			f=$( firmware_filename "$d" ) || {
 				# Fetching the CFILE here is often the
 				# first attempt to talk to FWURL
@@ -718,6 +793,7 @@ if [ "${devices[*]:-}" ]; then
 				continue
 			}
 			if [ ! "$f" ]; then
+				INFO="Unregistering firmware for $d"
 				if "$INSTALL" && unregister_firmware "$d"; then
 					unregister="$unregister,$d"
 				else
@@ -732,6 +808,7 @@ if [ "${devices[*]:-}" ]; then
 			# Don't verify files specified on the command-line
 			verify_existing=false
 		fi
+		INFO="Fetching ${f##*/} for $d"
 
 		if "$LIST"; then
 			echo "$FWURL/$f"
@@ -762,26 +839,37 @@ if [ "${devices[*]:-}" ]; then
 		fi
 
 		if "$verify_existing" && [ -e "$f" ]; then
+			INFO="Verifying ${f##*/}"
 			pending_status=false
 			if ((VERBOSE == 1)); then
-				echo -n "Verify ${f##*/} ..."
+				echo -n "Verify ${f##*/} ..." |
+				    tee "$LAST_STATUS"
 				pending_status=true
 			elif ((VERBOSE > 1)) && ! "$INSTALL"; then
 				echo "Keep/Verify ${f##*/}"
 			fi
 
 			if "$DRYRUN" || verify_existing "$f"; then
-				"$pending_status" && echo " done."
+				"$pending_status" && {
+				    echo " done."
+				    rm -f "$LAST_STATUS"
+				}
 				if ! "$INSTALL"; then
 					kept="$kept,$d"
 					continue
 				fi
 			elif "$DOWNLOAD"; then
-				"$pending_status" && echo " failed."
+				"$pending_status" && {
+				    echo " failed."
+				    rm -f "$LAST_STATUS"
+				}
 				((VERBOSE > 1)) && echo "Refetching $f"
 				rm -f "$f"
 			else
-				"$pending_status" && echo " failed."
+				"$pending_status" && {
+				    echo " failed."
+				    rm -f "$LAST_STATUS"
+				}
 				continue
 			fi
 		fi
@@ -825,13 +913,16 @@ for f in "${add[@]}" _update_ "${update[
 	status "$comma$d"
 	comma=,
 
+	INFO="${action%e}ing ${f##*/} for $d"
+
 	pending_status=false
 	if [ -e "$f" ]; then
 		if "$DRYRUN"; then
 			((VERBOSE)) && echo "$action ${f##*/}"
 		else
 			if ((VERBOSE == 1)); then
-				echo -n "Install ${f##*/} ..."
+				echo -n "Install ${f##*/} ..." |
+				    tee "$LAST_STATUS"
 				pending_status=true
 			fi
 		fi
@@ -840,14 +931,18 @@ for f in "${add[@]}" _update_ "${update[
 			((VERBOSE)) && echo "Get/Verify ${f##*/}"
 		else
 			if ((VERBOSE == 1)); then
-				echo -n "Get/Verify ${f##*/} ..."
+				echo -n "Get/Verify ${f##*/} ..." |
+				    tee "$LAST_STATUS"
 				pending_status=true
 			fi
 			fetch  "$f" &&
 			verify "$f" || {
 				integer e=$?
 
-				"$pending_status" && echo " failed."
+				"$pending_status" && {
+				    echo " failed."
+				    rm -f "$LAST_STATUS"
+				}
 				status " failed (${f##*/})"
 
 				if ((VERBOSE)) && [ -s "$FD_DIR/warn" ]; then
@@ -868,7 +963,10 @@ for f in "${add[@]}" _update_ "${update[
 	fi
 
 	if ! "$INSTALL"; then
-		"$pending_status" && echo " done."
+		"$pending_status" && {
+		    echo " done."
+		    rm -f "$LAST_STATUS"
+		}
 		continue
 	fi
 
@@ -876,6 +974,7 @@ for f in "${add[@]}" _update_ "${update[
 		if [ "$action" = Update ]; then
 			for i in $( installed_firmware '' "$d-firmware-" '*' )
 			do
+				INFO="Removing $i"
 				delete_firmware "$i" || {
 					"$pending_status" &&
 					    echo -n " (remove $i failed)"
@@ -887,8 +986,12 @@ for f in "${add[@]}" _update_ "${update[
 			done
 		fi
 
+		INFO="${action%e}ing ${f##*/}"
 		add_firmware "$f" "$action" || {
-			"$pending_status" && echo " failed."
+			"$pending_status" && {
+			    echo " failed."
+			    rm -f "$LAST_STATUS"
+			}
 			status " failed (${f##*/})"
 			continue
 		}
@@ -900,6 +1003,7 @@ for f in "${add[@]}" _update_ "${update[
 		else
 			echo " updated."
 		fi
+		rm -f "$LAST_STATUS"
 	fi
 done