nut: prevent service thrashing on boot or hotplug

On boot, hotplug events were causing excessive start and stop action
for the upsd daemon and driver daemons. We fix that with two primary
actions:
  1. Don't restart service daemons on hotplug until after first boot
     has completed.
  2. Use more robust handling of procd instance starts by ensuring
     that the first start starts the nut-server service and all
     others add to the nut-server service (rather than replacing it).

Ignore hotplug events without a DEVNAME.

In addition clean up some logging.

In the process, this fixes #30375 "hotplugging for setting usb access
right[s] doesn't work anymore"

Closes: #30375

Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>

net/nut/Makefile

@@ -9,7 +9,7 @@ include $(TOPDIR)/rules.mk
 
 PKG_NAME:=nut
 PKG_VERSION:=2.8.5
-PKG_RELEASE:=5
+PKG_RELEASE:=6
 
 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
 PKG_SOURCE_URL:=https://www.networkupstools.org/source/2.8/

net/nut/files/libhid-ups.hotplug

@@ -202,7 +202,8 @@ nut_driver_config() {
 			return 0
 		fi
 		if [ "$(printf "%04x" 0x"$pvendid")" = "$cfg_vendorid" ] &&
-			[ "$(printf "%04x" 0x"$pprodid")" = "$cfg_productid" ]; then
+			[ "$(printf "%04x" 0x"$pprodid")" = "$cfg_productid" ] &&
+			[ -n "$DEVNAME" ] && [ -n "$ups" ]; then
 			# Round 1: If we have a match for hotplug event and the UCI config
 			# start this UPS and record that fact, otherwise skip this UCI UPS
 			# for this round

net/nut/files/nut-serial.hotplug

@@ -162,8 +162,12 @@ nut_on_hotplug_add() {
 # Do not do any hotplug actions if hotplug is disabled. Not an error but log
 # for information
 if ! allow_hotplug_restart; then
-	# Informational and not an error
-	log_msg "Did not set permissions on serial port hotplug as NUT hotplug is disabled" nut-serial nut-serial-hotplug notice
+	# Don't spam logs on initial boot
+	# (before NUT_HOTPLUG_BOOT_COMPLETE_PATH exists)
+	if [ -f "$NUT_HOTPLUG_BOOT_COMPLETE_PATH" ]; then
+		# Informational and not an error
+		log_msg "Did not set permissions on serial port hotplug as NUT hotplug is disabled" nut-serial nut-serial-hotplug notice
+	fi
 	exit 0
 fi
 

net/nut/files/nut-server.init

@@ -14,7 +14,19 @@
 START=70
 STOP=30
 
-USE_PROCD=1
+# We use the script functions and variables definitions from
+# /etc/rc.common that are added by USE_PROCD except with a modified
+# start and rc_procd. We cannot simply override USE_PROCD's
+# implementation of start and rc_procd as rc.common sources this
+# initscript before the definitions are applied by USE_PROCD
+#
+# The modified rc_procd uses "set" for procd_close_service, which
+# registers the service with procd, if the service for this
+# initscript (nut-server) is not registered with procd (active) at all,
+# otherwise it uses "add" which adds an instance to the registered
+# (active) service.
+#
+# USE_PROCD=1
 
 # IPKG_INSTROOT is intentionally only set when building an image and
 # is intentionally empty on a live OpenWrt device
@@ -73,32 +85,70 @@ USE_PROCD=1
 	exit 1
 }
 
+# shellcheck source=/dev/null
+. "${IPKG_INSTROOT}"/lib/functions/procd.sh || {
+	log_source_error "procd.sh" "nut-server" "nut-server"
+	exit 1
+}
+
 restore_umask_on_exit
 
-# Start a ups driver instance
-start_ups_driver() {
-	local ups="$1"
-	local requested="$2"
-	local driver=""
+extra_command "running" "Check if service is running"
+extra_command "status" "Service status"
+extra_command "trace" "Start with syscall trace"
+extra_command "info" "Dump procd service info"
 
-	# If wanting a specific instance, only start that instance
-	if [ -n "$requested" ] && [ "$requested" != "$ups" ]; then
-		return 0
+basescript=$(readlink "$initscript")
+
+has_running_driver() {
+	local instances instance
+
+	instances="$(list_running_instances "nut-server")"
+	[ -n "$instances" ] || return 0
+	set -f
+	for instance in $instances; do
+		if [ "$instance" = "upsd" ]; then
+			have_upsd_instance="true"
+			continue
+		fi
+		have_driver_instance="true"
+		break
+	done
+	set +f
+}
+
+start_server_service() {
+	procd_open_instance upsd
+	procd_set_param respawn
+	procd_set_param stderr 1
+	procd_set_param stdout 1
+	procd_set_param env NUT_DEBUG_SYSLOG="stderr"
+	procd_set_param env NUT_QUIET_INIT_UPSNOTIFY=true
+	procd_set_param env NUT_STATEPATH="$STATEPATH"
+	procd_set_param command /usr/sbin/upsd
+	procd_append_param command -FF
+	procd_append_param command -u "$RUNAS"
+	procd_close_instance
+}
+
+# Start upsd instance
+start_server_instance() {
+	if [ -f "$NUT_KILLPOWER" ]; then
+		return 1
 	fi
 
-	# For non-USB devices this is effectively a no-op
-	# For USB devices, if the USB productid and USB vendorid of the configured
-	# driver instance (UPS) match a USB device in sysfs, set the permissions
-	# on the USB device node in /dev/bus/usb/... to allow NUT to access and
-	# control the device.
-	ensure_usb_ups_access "$ups" || return 1
-	config_get driver "$ups" driver
+	local have_driver_instance="false"
+	local have_upsd_instance="false"
+	has_running_driver
 
-	[ -n "$driver" ] || {
-		log_error "Missing driver when starting UPS '$ups'" "nut-server" "nut-server"
-		return 1
-	}
+	# upsd will crash if started with no available UPS, so only start if
+	# we have one, and we do not already have a upsd instance
+	if [ "$have_driver_instance" = "true" ] && [ "$have_upsd_instance" = "false" ]; then
+		rc_procd start_server_service || return 1
+	fi
+}
 
+start_ups_service() {
 	procd_open_instance "$ups"
 	procd_set_param respawn
 	procd_set_param stderr 1
@@ -110,22 +160,42 @@ start_ups_driver() {
 	procd_append_param command -FF -a "$ups"
 	procd_append_param command -u "$RUNAS"
 	procd_close_instance
-
-	# If upsd is running, reload it to pick up new driver
-	if procd_running "nut-server" "upsd" >/dev/null 2>&1; then
-		signal_instance "upsd" "upsd" "reload" "HUP" "${STATEPATH}/upsd.pid" "" "nut-server"
-	else
-		start_server_instance
-	fi
 }
 
-# On reload, we need to use rpc_procd to add an instance to an already running
-# service, as procd_open_service is not called by default when doing a
-# reload vs start
-add_ups_driver() {
+# Start a ups driver instance
+start_ups_driver() {
 	local ups="$1"
 	local requested="$2"
-	rc_procd start_ups_driver "$ups" "$requested"
+	local driver=""
+
+	# If wanting a specific instance, only start that instance; this is a normal
+	# operating event, so do not log and do not error
+	if [ -n "$requested" ] && [ "$requested" != "$ups" ]; then
+		return 0
+	fi
+
+	# For non-USB devices this is effectively a no-op
+	# For USB devices, if the USB productid and USB vendorid of the configured
+	# driver instance (UPS) match a USB device in sysfs, set the permissions
+	# on the USB device node in /dev/bus/usb/... to allow NUT to access and
+	# control the device.
+	ensure_usb_ups_access "$ups" || {
+		usb_ups_access_error="true"
+		log_error "Error enabling USB access for UPS '$ups'" "nut-server" "nut-server"
+		return 1
+	}
+
+	config_get driver "$ups" driver
+
+	[ -n "$driver" ] || {
+		log_error "Missing driver when starting UPS '$ups'" "nut-server" "nut-server"
+		return 1
+	}
+
+	rc_procd start_ups_service "$@" || {
+		return 1
+	}
+	return 0
 }
 
 have_upsd_section() {
@@ -170,58 +240,52 @@ common_preconditions() {
 stop_no_longer_configured_instances() {
 	local have_driver_instance=false
 	local have_upsd_instance=false
+	local had_running_instance="false"
 	local instance instances
 
 	# Stop any driver instances which are no longer configured
 	# We can only reliably do this for instances managed by procd
-	set -f
 	instances="$(list_running_instances "nut-server")"
 	[ -n "$instances" ] || return 0
+	set -f
 	for instance in $instances; do
 		if [ "$instance" = "upsd" ]; then
 			continue
 		fi
+		had_running_instance="true"
 		config_get driver "$instance" driver
 		# Only stop not configured but running instances
 		if [ -z "$driver" ] && [ -n "$instance" ] && procd_running "nut-server" "$instance" >/dev/null 2>&1; then
-			procd_kill "nut-server" "$instance" 2>&1 | logger -t nut-server
+			log_msg "Stopping no longer configured ups '$instance'" "nut-server" "nut-server" notice
+			procd_kill "nut-server" "$instance"
 		fi
 	done
 	set +f
 
 	# Need to stop drivers before stopping upsd
-	# The second loop is required in case some instances failed to stop in the
-	# first loop (so we cannot just track instances in the above loop).
-	# In addition, it is possible for hotplug triggered drivers start to start
+	# The has_running_driver check is required in case some instances failed to
+	# stop in the first loop (so we cannot just track instances in the above loop).
+	# In addition, it is possible for hotplug triggered drivers starting to start
 	# instances between the stop above, and this loop.
-	set -f
-	instances="$(list_running_instances "nut-server")"
-	[ -n "$instances" ] || return 0
-	for instance in $instances; do
-		if [ "$instance" = "upsd" ]; then
-			have_upsd_instance="true"
-			continue
-		fi
-		have_driver_instance="true"
-		break
-	done
-	set +f
+	has_running_driver
 
 	# If we have no UPS instances we must stop upsd or it will crash
 	# The "nut-server" service remains active and will 'see' configuration
 	# changes and execute reload_service when they are detected
-	if [ "$have_upsd_instance" = "true" ] && [ "$have_driver_instance" = "false" ]; then
-		# stop_server_instance
+	if [ "$have_upsd_instance" = "true" ] && [ "$have_driver_instance" = "false" ] && [ "$had_running_instance" = "true" ]; then
+		log_msg "Stopping upsd because no driver instances configured" "nut-server" "nut-server" "notice"
 		signal_instance "upsd" "upsd" "stop" "TERM" "${STATEPATH}/upsd.pid" "" "nut-server" "procd_kill" "nut-server" "upsd"
+	elif [ "$have_upsd_instance" = "false" ] && [ "$have_driver_instance" = "true" ]; then
+		start_server_instance
 	fi
 }
 
 stop_all_instances() {
 	local instance instances
 
-	set -f
 	instances="$(list_running_instances "nut-server")"
 	[ -n "$instances" ] || return 0
+	set -f
 	for instance in $instances; do
 		# stop upsd last
 		if [ "$instance" = "upsd" ]; then
@@ -305,25 +369,6 @@ service_preconditions() {
 	return 0
 }
 
-# Start upsd instance
-start_server_instance() {
-	if [ -f "$NUT_KILLPOWER" ]; then
-		return 1
-	fi
-
-	procd_open_instance upsd
-	procd_set_param respawn
-	procd_set_param stderr 1
-	procd_set_param stdout 1
-	procd_set_param env NUT_DEBUG_SYSLOG="stderr"
-	procd_set_param env NUT_QUIET_INIT_UPSNOTIFY=true
-	procd_set_param env NUT_STATEPATH="$STATEPATH"
-	procd_set_param command /usr/sbin/upsd
-	procd_append_param command -FF
-	procd_append_param command -u "$RUNAS"
-	procd_close_instance
-}
-
 remove_var_config() {
 	for rm_file in "$USERS_C" "$UPS_C" "$UPSD_C" \
 		"$USERS_C.new" "$UPS_C.new" "$UPSD_C.new"; do
@@ -390,8 +435,12 @@ reload_ups_driver() {
 	# If the driver instance (UPS) was terminated, or stopped 'naturally',
 	# start it up again
 	if ! procd_running "nut-server" "$ups" >/dev/null 2>&1; then
-		add_ups_driver "$ups" "$ups"
+		start_ups_driver "$ups" "$ups"
 	fi
+
+	# If the server instance (upsd) was terminated, or stopped 'naturally',
+	# start it up again
+	start_server_instance
 	return 0
 }
 
@@ -412,14 +461,13 @@ manage_instances() {
 	case "$action" in
 	start)
 		config_foreach start_ups_driver driver
+		start_server_instance
 		;;
 	reload)
 		if service_active_no_instances "nut-server"; then
 			log_msg "nut-server active with no instances. Reloading." "nut-server" "nut-server" "info"
 		fi
-		if procd_running "nut-server" upsd >/dev/null 2>&1; then
-			signal_instance "upsd" "upsd" "reload" "HUP" "${STATEPATH}/upsd.pid" "" "nut-server" "procd_kill" "nut-server" "upsd"
-		fi
+
 		config_foreach reload_ups_driver driver
 		stop_no_longer_configured_instances
 		;;
@@ -444,16 +492,14 @@ manage_service() {
 	# instances, we act only on one instance).
 	local instance="$1"
 	local ret=0
+	local service_active
 
 	case "$action" in
 	start)
+		service_preconditions "$action"
+		ret=$?
 		case "$instance" in
 		"")
-			service_preconditions "$action"
-			ret=$?
-
-			stop_no_longer_configured_instances
-
 			if [ "$ret" = "0" ]; then
 				manage_instances start
 			else
@@ -462,27 +508,20 @@ manage_service() {
 			;;
 		# We only start one service (upsd or one driver) from a given invocation
 		upsd)
-			service_preconditions "$action"
-			ret=$?
 			if [ "$ret" = "0" ]; then
 				start_server_instance
 			fi
-			stop_no_longer_configured_instances
-
-			return $ret
 			;;
 		# We only start one service (upsd or one driver) from a given invocation
 		*)
-			service_preconditions "$action"
-			ret=$?
 			if [ "$ret" = "0" ]; then
 				config_foreach start_ups_driver driver "$instance"
+				start_server_instance
 			fi
-			stop_no_longer_configured_instances
-
-			return $ret
 			;;
 		esac
+		stop_no_longer_configured_instances
+		return $ret
 		;;
 	reload)
 		service_preconditions "$action"
@@ -492,8 +531,6 @@ manage_service() {
 			manage_instances reload
 		fi
 
-		stop_no_longer_configured_instances
-
 		if [ "$ret" = "1" ]; then
 			return 1
 		fi
@@ -502,17 +539,27 @@ manage_service() {
 		case "$instance" in
 		"")
 			manage_instances stop
+			ret=$?
+
+			service_active_no_instances "$(basename "${basescript:-$initscript}")"
+			service_active=$?
+
+			if [ "$service_active" -eq 1 ]; then
+				# Prevent NUT hotplug events when service is not running
+				rm -f "$NUT_HOTPLUG_BOOT_COMPLETE_PATH"
+			elif [ "$service_active" -eq 3 ]; then
+				return 1
+			fi
+			return $ret
 			;;
 		upsd)
 			if procd_running "nut-server" upsd >/dev/null 2>&1; then
 				signal_instance "upsd" "upsd" "stop" "TERM" "${STATEPATH}/upsd.pid" "" "nut-server" "procd_kill" "nut-server" "upsd"
 			fi
-			stop_service_if_no_instances
 			;;
 		*)
 			# We only handle the first parameter, so do not pass in all parameters
 			config_foreach stop_ups_driver driver "$instance"
-			stop_service_if_no_instances
 			;;
 		esac
 		;;
@@ -535,14 +582,61 @@ manage_service() {
 	esac
 }
 
+allow_hotplug() {
+	local ret="$1"
+
+	[ "$ret" -eq 0 ] || return "$ret"
+
+	# Allow hotplug events to (re)start drivers/upsd only if starting
+	# nut-server service was successful
+	touch "$NUT_HOTPLUG_BOOT_COMPLETE_PATH"
+
+	# Ensure drivers/upsd started once hotplug has been allowed
+	local usb_ups_access_error="false"
+	config_foreach start_ups_driver driver
+	if [ "$usb_ups_access_error" = "false" ]; then
+		start_server_instance
+		ret=$?
+	else
+		ret=1
+	fi
+
+	# Re-disable hotplug if starting drivers and/or upsd failed after
+	# enabling hotplug.
+	if [ "$ret" -ne 0 ]; then
+		rm -f "$NUT_HOTPLUG_BOOT_COMPLETE_PATH"
+	fi
+
+	return "$ret"
+}
+
 # Start NUT drivers and upsd (NUT server)
 start_service() {
+	local ret
 	manage_service start "$@"
+	ret=$?
+
+	# Only change allowing hotplug events for the
+	# start all instances case
+	if [ -z "$1" ]; then
+		allow_hotplug "$ret"
+	fi
 }
 
 # Reload NUT drivers and upsd (NUT server)
 reload_service() {
+	local ret
+
 	manage_service reload "$@"
+	ret=$?
+
+	# Only change allowing hotplug events for the
+	# reload all instances case
+	if [ -z "$1" ]; then
+		# Allow hotplug events to (re)start drivers/upsd when reloading
+		# (successfully) after a failed start
+		allow_hotplug "$ret"
+	fi
 }
 
 # Stop NUT drivers and upsd (NUT server)
@@ -568,3 +662,90 @@ service_triggers() {
 	}
 	procd_add_reload_trigger "nut_server"
 }
+
+rc_procd() {
+	local method service_active
+
+	service_active_no_instances "$(basename "${basescript:-$initscript}")"
+	service_active=$?
+
+	# Always use set if nut-server service is not active, even if this
+	# function is executed from reload or restart. In the default
+	# implementation from USE_PROCD=1, rc_procd is only called from
+	# start, so set is only used on a start action. In addition, the
+	# default rc_procd only uses add when called with a second parameter
+	if [ "$service_active" -eq 1 ]; then
+		log_msg "$(basename "${basescript:-$initscript}") was not active, so start it" "nut-server" "nut-server" "debug"
+		method="set"
+	elif [ "$service_active" -eq 3 ]; then
+		# log error message was already issued in service_active_no_instances
+		return 1
+	elif [ "$service_active" -eq 0 ] || [ "$service_active" -eq 2 ]; then
+		method="add"
+	else
+		log_error "Unexpected return value from 'service_active_no_instances'" "nut-server" "nut-server"
+		return 1
+	fi
+	procd_open_service "$(basename "${basescript:-$initscript}")" "$initscript"
+	"$@"
+	procd_close_service "$method"
+
+	return 0
+}
+
+# Define start in a way that avoids nesting rc_procd
+start() {
+	# Ensure start actions occur one at a time
+	procd_lock
+
+	# Ensure service is started even if no instances are started
+	if rc_procd :; then
+		start_service "$@"
+	fi
+}
+
+boot() {
+	start
+}
+
+trace() {
+	TRACE_SYSCALLS=1
+	start "$@"
+}
+
+info() {
+	json_init
+	json_add_string name "$(basename "${basescript:-$initscript}")"
+	json_add_boolean verbose "1"
+	_procd_ubus_call list
+}
+
+stop() {
+	procd_lock
+	stop_service "$@"
+	procd_kill "$(basename "${basescript:-$initscript}")" "$1"
+	if eval "type service_stopped" 2>/dev/null >/dev/null; then
+		service_stopped
+	fi
+}
+
+reload() {
+	if eval "type reload_service" 2>/dev/null >/dev/null; then
+		procd_lock
+		reload_service "$@"
+	else
+		start
+	fi
+}
+
+running() {
+	service_running "$@"
+}
+
+status() {
+	if eval "type status_service" 2>/dev/null >/dev/null; then
+		status_service "$@"
+	else
+		_procd_status "$(basename "${basescript:-$initscript}")" "$1"
+	fi
+}

net/nut/files/nut-service.sh.functions

@@ -21,6 +21,8 @@ NUT_KILLPOWER="/var/run/killpower"
 
 # Disable NUT hotplug path
 NUT_DISABLE_HOTPLUG_PATH="/var/run/nut/disable-hotplug"
+# Must be present to allow hotplug
+NUT_HOTPLUG_BOOT_COMPLETE_PATH="/var/run/nut-hotplug-boot-complete"
 
 # Fallback STATEPATH setting
 NUT_BASE_STATEPATH="/var/run/nut"
@@ -48,6 +50,9 @@ allow_hotplug_restart() {
 	# or nutshutdown
 	[ ! -f "$NUT_DISABLE_HOTPLUG_PATH" ] || return 1
 
+	# Initial boot complete; NUT hotplug allowed sentinel
+	# Created by nut-server initscript
+	[ -f "$NUT_HOTPLUG_BOOT_COMPLETE_PATH" ] || return 1
 	return 0
 }
 
@@ -211,14 +216,23 @@ find_runas() {
 	) || return $?
 }
 
-# Detect if service is running under procd, with no instances
-# (e.g. upsd with no UPS drivers running)
+# Detect if a service is active with procd, and if it has
+# any instances.
+#
+# Returns:
+#  0 - service is registered (active) but has no instances
+#  1 - service is not registered (active) at all
+#  2 - service is registered (active) and has instances
+#  3 - error condition encountered
 service_active_no_instances() {
 	local service="$1"
 	local active_instances
 	local active_var service_filter
 
-	check_safe_uci_name "$service" || return 1
+	check_safe_uci_name "$service" || {
+		log_error "Invalid service name '$service' in service_active_no_instances" nut-service.sh nut-service
+		return 3
+	}
 
 	service_filter="@['""${service}""']"
 	active_instances="$(_procd_ubus_call list "{\"name\":\"$service\"}" | jsonfilter -l 1 -e active_var="$service_filter")"
@@ -232,14 +246,16 @@ service_active_no_instances() {
 		;;
 	*)
 		log_error "Unexpected value '$active_instances' for 'active_instances' from jsonfilter in service_active_no_instances" nut-service.sh nut-service
-		return 1
+		return 3
 		;;
 	esac
 
 	# active_var is only empty, but present, if there are no instances for the service
 	[ -z "$active_var" ] && return 0
 
-	return 1
+	# otherwise there are instances, in any state, for the service; to check if
+	# any are running use procd_running
+	return 2
 }
 
 # Setup triggers in procd for changes to specified network interfaces