nut: ensure correct settings names

[danielfdickinson]

📦 Package Details

Maintainer: @danielfdickinson
Also-mention: @systemcrash @hnyman @mhei @usamanassir

Description:

• Prompted by luci-app-nut: fix sync between LuCI and init.d parameters luci#8420 (comment)
  we update upsmon configs to ensure they are correct according to upstream.
  We reorder the options so that they match upstream documentation at
  https://networkupstools.org/docs/man/upsmon.conf.html to be sure we have
  not missed any items. While at it, we add configuration options from the upstream documentation
  that are not currently present in the UCI configs.
• Some years ago upstream changed the names of the primary/secondary
  UPS system/monitor from master/slave to primary/secondary. It is uncertain
  how much longer these deprecated names will be accepted by NUT.
  Therefore update naming to match upstream documentation and configuration.
  See https://networkupstools.org/docs/man/upsmon.html,
  https://networkupstools.org/docs/man/upsmon.conf.html, and
  https://networkupstools.org/docs/man/upsd.users.html.
• Add myself as maintainer, and bump PKG_RELEASE for this PR series.

---

🧪 Run Testing Details

• OpenWrt Version: OpenWrt SNAPSHOT r33436-7d2c244646
• OpenWrt Target/Subtarget: bcm27xx/bcm2709
• OpenWrt Device: Raspberry Pi 2 Model B Rev 1.1

UPS: Tripp-Lite ECO550U
UPS: Tripp-Lite AVR550UPS
UPS: dummy-ups

---

✅ Formalities

• I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

[geldot]

I feel the repetition has grown out of hand and prone to copy-paste typos.

Can we refactor this to just keep a list of notification names, then loop through them, deriving the config name via lowercase and s/_//?

e.g. in your patch the setting name is suspendstaringmsg without a t but the official event name is SUSPEND_STARTING:

config_get val "$cfg" suspendstaringmsg
	[ -n "$val" ] && echo "NOTIFYMSG SUSPEND_STARTING \"$val\"" >> "$UPSMON_C"

(In fact, my preference would be to deprecate this entirely and switch to an approach that uses the upstream names without alteration, e.g. a list of tuples, now that LuCi has validation support, but that'd be a bigger change.)

[danielfdickinson]

@geldot I think refactoring is a good idea; I don't know if I will have the time to implement all that soon.

[geldot]

@geldot I think refactoring is a good idea; I don't know if I will have the time to implement all that soon.

OK. The patch is full of typos, and I see more mistakes in there each time I look e.g. notlarmmsg.

This should be switched to draft until someone has a minute to write the three lines of shell script to automate it, because what's there clearly is no longer humanly maintainable.

(Imagine the technical debt of maintaining backward compatibility for each of these typos.)

[geldot]

You can do the below. It preserves the existing approach without the copypasta.

Please fix openwrt/luci#8434 to de-duplicate the parsing as the new monitor section and the legacy master/slave are equivalent.

(In the long run, these weird message and flag hacks should be eliminated and replaced with free-form messages and flags sections. That way (a) OpenWRT won't need to be manually updated after every upstream change and (b) unknown or misspelled flag names will raise an error instead of being silently ignored, which is a bad failure mode for something that's not difficult to test like power outages.)

diff --git c/net/nut/files/nut-monitor.init w/net/nut/files/nut-monitor.init
index 9b17d42e5..e6f2b7b03 100755
--- c/net/nut/files/nut-monitor.init
+++ w/net/nut/files/nut-monitor.init
@@ -9,6 +9,9 @@ USE_PROCD=1
 UPSMON_C=/var/etc/nut/upsmon.conf
 PIDFILE=/var/run/upsmon.pid
 
+# Upstream NUT event type names we map to UCI options.
+NUT_EVENT_TYPES="ONLINE ONBATT LOWBATT FSD COMMOK COMMBAD SHUTDOWN REPLBATT NOCOMM NOPARENT"
+
 nut_upsmon_conf() {
 	local cfg="$1"
 	local RUNAS val optval
@@ -42,67 +45,22 @@ nut_upsmon_conf() {
 
 	echo "POWERDOWNFLAG /var/run/killpower" >> "$UPSMON_C"
 
-	config_get val "$cfg" onlinemsg
-	[ -n "$val" ] && echo "NOTIFYMSG ONLINE \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" onbattmsg
-	[ -n "$val" ] && echo "NOTIFYMSG ONBATT \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" lowbattmsg
-	[ -n "$val" ] && echo "NOTIFYMSG LOWBATT \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" fsdmsg
-	[ -n "$val" ] && echo "NOTIFYMSG FSD \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" commokmsg
-	[ -n "$val" ] && echo "NOTIFYMSG COMMOK \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" commbadmsg
-	[ -n "$val" ] && echo "NOTIFYMSG COMMBAD \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" shutdownmsg
-	[ -n "$val" ] && echo "NOTIFYMSG SHUTDOWN \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" replbattmsg
-	[ -n "$val" ] && echo "NOTIFYMSG REPLBATT \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" nocommmsg
-	[ -n "$val" ] && echo "NOTIFYMSG NOCOMM \"$val\"" >> "$UPSMON_C"
-	config_get val "$cfg" noparentmsg
-	[ -n "$val" ] && echo "NOTIFYMSG NOPARENT \"$val\"" >> "$UPSMON_C"
-
-	notifylist() {
-		local value="$1"
-		append optval "$value" "+"
+	nut_to_uci_key() {
+		echo "$1" | tr '[:upper:]' '[:lower:]' | tr -d '_'
 	}
 
-	setnotify() {
-		local cfg="$1"
-		local optname="$2"
-		local optval
-		config_list_foreach "$cfg" "$optname" notifylist
-		if [ -z "$optval" ]; then
-			# If no list fallback to trying option, fallback to default
-			config_get optval "$cfg" "$optname" "$default"
-			if [ -n "$optval" ]; then
-				echo "$optval"
-			else
-				# No default, so do the NUT default
-				echo "SYSLOG"
-			fi
-		else
-			echo "$optval"
-		fi
-        }
+	for event in $NUT_EVENT_TYPES; do
+		config_get val "$cfg" "$(nut_to_uci_key "$event")msg"
+		[ -n "$val" ] && echo "NOTIFYMSG $event \"$val\"" >> "$UPSMON_C"
+	done
 
 	local default optval
-	val=""
-	config_list_foreach "$cfg" defaultnotify notifylist
-	default="$optval"
-	{
-		echo "NOTIFYFLAG ONLINE $(setnotify "$cfg" onlinenotify)" ; \
-		echo "NOTIFYFLAG ONBATT $(setnotify "$cfg" onbattnotify)" ; \
-		echo "NOTIFYFLAG LOWBATT $(setnotify "$cfg" lowbatnotify)" ; \
-		echo "NOTIFYFLAG FSD $(setnotify "$cfg" fsdnotify)" ; \
-		echo "NOTIFYFLAG COMMOK $(setnotify "$cfg" commoknotify)" ; \
-		echo "NOTIFYFLAG COMMBAD $(setnotify "$cfg" commbadnotify)" ; \
-		echo "NOTIFYFLAG SHUTDOWN $(setnotify "$cfg" shutdownnotify)" ; \
-		echo "NOTIFYFLAG REPLBATT $(setnotify "$cfg" replbattnotify)" ; \
-		echo "NOTIFYFLAG NOCOMM $(setnotify "$cfg" nocommnotify)" ; \
-		echo "NOTIFYFLAG NOPARENT $(setnotify "$cfg" noparentnotify)" ; \
-	} >> "$UPSMON_C"
+	config_get default "$cfg" defaultnotify "SYSLOG"
+	for event in $NUT_EVENT_TYPES; do
+		config_get optval "$cfg" "$(nut_to_uci_key "$event")notify" "$default"
+		optval="${optval// /+}"
+		echo "NOTIFYFLAG $event $optval"
+	done >> "$UPSMON_C"
 
 	config_get val "$cfg" rbwarntime 43200
 	echo "RBWARNTIME $val" >> "$UPSMON_C"
@@ -142,6 +100,8 @@ nut_upsmon_add() {
 	config_get powervalue "$cfg" powervalue 1
 	config_get username "$cfg" username
 	config_get password "$cfg" password
+	config_get type "$cfg" type "${type:-secondary}"
+
 	system="$upsname@$hostname"
 	if [ -n "$port" ]; then
 	    system="$system:$port";
@@ -157,6 +117,8 @@ build_config() {
 
 	config_load nut_monitor
 	config_foreach nut_upsmon_conf upsmon
+	config_foreach nut_upsmon_add monitor ""
+	# master and slave are legacy section names. Prefer monitor.
 	config_foreach nut_upsmon_add master master
 	config_foreach nut_upsmon_add slave slave
 
diff --git c/net/nut/files/nut_monitor w/net/nut/files/nut_monitor
index a3de4f5de..c1e9c04f8 100644
--- c/net/nut/files/nut_monitor
+++ w/net/nut/files/nut_monitor
@@ -9,25 +9,7 @@
 # 	option hostsync 15
 #	option deadtime 15
 #	option onlinemsg "online message"
-#	option onbattmsg "on battery message"
-#	option lowbattmsg "low battery message"
-#	option fsdmsg "forced shutdown message"
-#	option comokmsg "communications restored message"
-#	option combadmsg "communications bad message"
-#	option shutdowmsg "shutdown message"
-#	option replbattmsg "replace battery message"
-#	option nocommmsg "no communications message"
-#	option noparentmsg "no parent message"
 #	list onlinenotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list onbattnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list lowbattnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list fsdnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list comoknotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list combadnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list shutdownotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list replbattnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list nocommnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
-#	list noparentnotify SYSLOG # can be SYSLOG or EXEC or IGNORE
 #	option rbwarntime 4200 # replace battery warn time
 #	option nocommwarntime 300 # no communications warn time
 #	option finaldelay 5 # final delay
@@ -35,7 +17,8 @@
 #	option certverify 0
 #	option forcessl 0
 
-#config master
+#config monitor
+#	option type primary
 #	option upsname upsname
 #	option hostname localhost
 #	option port # optional port number
@@ -43,7 +26,8 @@
 #	option username upsuser
 #	option password upspassword
 
-#config slave
+#config monitor
+#	option type secondary
 #	option upsname upsname
 #	option hostname localhost
 #	option port # optional port number

[danielfdickinson]

@geldot Thanks for that. Been sick, but will try to run it through its paces this weekend.

[geldot]

I'm reading the git history and there have been so many misspellings in this list, I now think the primary/secondary -> monitor change is an opportunity for a clean break.

Under this proposal, we would drop the mappings entirely, and leave two trivial freeform sections as an escape hatch for advanced users:

config monitor
	option name apc1
	option type primary

config monitor
	option name apc2
	option type secondary

# Advanced customizations

config messages
  option COMMOK "Communications with UPS %s established"
config flags
  list COMMOK "SYSLOG"

The rationale is that:

1. The list of events, strings, and substitution patterns, and translations changes upstream frequently.
2. User-customized messages are a niche, basically intended for 80s-style multi-user UNIX systems for broadcast via wall, not a router, and this is within a package which is itself somewhat niche.
3. The hard-coded lists in the init script and LuCi are full of typos so it's likely any existing user configs are already silently not working.
4. It doesn't make sense to force the LuCi frontend to maintain and translate dozens of UPS message strings, which include complex runtime substitution patterns, in dozens of languages. It's a combinatorial explosion of human effort and device storage space serving no obvious purpose.

So basically, let's drop the master/slave sections which my patch already can do, and drop all the event string mappings, hence eliminating all the LuCi code duplication that handles them. Instead, just leave config monitor plus the messages and flags escape hatches for power users.

This way it will never break again, everything is simplified, hundreds of outdated translations can be removed, and now config errors are detected at init time instead of ignored.

It all collapses down to three or four lines of shell script, and a few lines of JS in the frontend instead of what's there now.

[danielfdickinson]

@geldot Sorry, not 'tracking' that well right now due to illness. Since this would have to be approved by repo committers anyway, pinging a couple of them for their thoughts, as well as possibly interested recent users. It sounds like a good idea to me, FWIW.

@systemcrash @hnyman @mhei @rpavlik @usamanassir

[mhei]

Sounds reasonable for me, too.