Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: Better SIGINFO for fw_update(8)
To:
Andrew Hewus Fresh <andrew@afresh1.com>
Cc:
tech@openbsd.org
Date:
Tue, 28 Oct 2025 11:05:12 +0100

Download raw body.

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

To me this all seems a lot of complexity for little gain...

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