Index | Thread | Search

From:
Andrew Hewus Fresh <andrew@afresh1.com>
Subject:
Re: Better SIGINFO for fw_update(8)
To:
tech@openbsd.org
Date:
Mon, 27 Oct 2025 20:39:46 -0700

Download raw body.

Thread
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.


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	28 Oct 2025 03:34:35 -0000
@@ -46,8 +46,11 @@ unset FWPKGTMP
 REMOVE_LOCALSRC=false
 DROP_PRIVS=true
 
-status() { echo -n "$*" >&"$STATUS_FD"; }
-warn()   { echo    "$*" >&"$WARN_FD"; }
+status() { 
+	[ ! "${FD_DIR:-}" ] || echo -n "$*" >> "$FD_DIR/last_status"
+	echo -n "$*" >&"$STATUS_FD"
+}
+warn() { echo "$*" >&"$WARN_FD"; }
 
 cleanup() {
 	set +o errexit # ignore errors from killing ftp
@@ -101,6 +104,42 @@ spin() {
 	}>/dev/tty
 }
 
+ftp_info() {
+	local _src="$1" _ftp_errors="$2"
+	local _timeout=30 # tenths of a second
+
+	# If there was a last_status start the info on the next line
+	# and maybe overwrite the spinner.
+	[ ! "${FD_DIR:-}" ] || [ ! -s "$FD_DIR/last_status" ] || echo " "
+
+	echo "Fetching $_src" >&2
+
+	kill -INFO -"$FTPPID" 2>/dev/null || return 0
+
+	# If VERBOSE is than 2, we are storing its STDERR in _ftp_errors,
+	# and we need special handling, otherwise it outputs as normal.
+	(( VERBOSE < 2 )) || return 0
+
+        # Normally in this case ftp is quiet until it prints INFO
+        # so give up after a while in case it never shows up.
+	# 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"
+
+	# Re-print the last_status so the line looks right.
+	[ ! "${FD_DIR:-}" ] || [ ! -s "$FD_DIR/last_status" ] ||
+	    cat "$FD_DIR/last_status"
+
+	return 0
+}
+
 fetch() {
 	local _src="${FWURL}/${1##*/}" _dst=$1 _user=_file _exit _error=''
 	local _ftp_errors="$FD_DIR/ftp_errors"
@@ -127,6 +166,9 @@ fetch() {
 	) & FTPPID=$!
 	set +o monitor
 
+	trap 'ftp_info "$_src" "$_ftp_errors"' INFO
+	stty nokerninfo
+
 	SECONDS=0
 	_last=0
 	while kill -0 -"$FTPPID" 2>/dev/null; do
@@ -150,6 +192,8 @@ fetch() {
 	_exit=$?
 	set -o errexit
 
+	stty kerninfo
+	trap - INFO
 	unset FTPPID
 
 	if ((_exit != 0)); then
@@ -764,24 +808,28 @@ if [ "${devices[*]:-}" ]; then
 		if "$verify_existing" && [ -e "$f" ]; then
 			pending_status=false
 			if ((VERBOSE == 1)); then
-				echo -n "Verify ${f##*/} ..."
+				echo -n "Verify ${f##*/} ..." |
+				    tee "$FD_DIR/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 "$FD_DIR/last_status"
 				if ! "$INSTALL"; then
 					kept="$kept,$d"
 					continue
 				fi
 			elif "$DOWNLOAD"; then
-				"$pending_status" && echo " failed."
+				"$pending_status" && echo " failed." &&
+				    rm -f "$FD_DIR/last_status"
 				((VERBOSE > 1)) && echo "Refetching $f"
 				rm -f "$f"
 			else
-				"$pending_status" && echo " failed."
+				"$pending_status" && echo " failed." &&
+				    rm -f "$FD_DIR/last_status"
 				continue
 			fi
 		fi
@@ -831,7 +879,8 @@ for f in "${add[@]}" _update_ "${update[
 			((VERBOSE)) && echo "$action ${f##*/}"
 		else
 			if ((VERBOSE == 1)); then
-				echo -n "Install ${f##*/} ..."
+				echo -n "Install ${f##*/} ..." |
+				    tee "$FD_DIR/last_status"
 				pending_status=true
 			fi
 		fi
@@ -840,14 +889,16 @@ 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 "$FD_DIR/last_status"
 				pending_status=true
 			fi
 			fetch  "$f" &&
 			verify "$f" || {
 				integer e=$?
 
-				"$pending_status" && echo " failed."
+				"$pending_status" && echo " failed." &&
+				    rm -f "$FD_DIR/last_status"
 				status " failed (${f##*/})"
 
 				if ((VERBOSE)) && [ -s "$FD_DIR/warn" ]; then
@@ -868,7 +919,8 @@ for f in "${add[@]}" _update_ "${update[
 	fi
 
 	if ! "$INSTALL"; then
-		"$pending_status" && echo " done."
+		"$pending_status" && echo " done." &&
+		    rm -f "$FD_DIR/last_status"
 		continue
 	fi
 
@@ -888,7 +940,8 @@ for f in "${add[@]}" _update_ "${update[
 		fi
 
 		add_firmware "$f" "$action" || {
-			"$pending_status" && echo " failed."
+			"$pending_status" && echo " failed." &&
+			    rm -f "$FD_DIR/last_status"
 			status " failed (${f##*/})"
 			continue
 		}
@@ -900,6 +953,7 @@ for f in "${add[@]}" _update_ "${update[
 		else
 			echo " updated."
 		fi
+		rm -f "$FD_DIR/last_status"
 	fi
 done