Merge pull request #2934 from desertwitch/issue-2928-nolegacy

dstate: clean separation of older UPS-status based ALARM and modern alarm_*-driven ALARM

Author: jimklimov

AUTHORS

@@ -196,6 +196,12 @@ E: [email protected]
 D: Updating device support docs after unnecessarily writing a redundant
 D: driver first
 
+N: desertwitch
+E: [email protected]
+D: Primarily contributions to upsmon, PR discussions, as well as small-scale
+D: QOL improvements throughout both UPS driver code and other project parts.
+P: 4096R/06FD918F FDE5 DBCF 1792 4276 532B FBCA DDA4 D1A9 06FD 918F
+
 N: Marco Trevisan (Treviño)
 E: [email protected]
 W: https://3v1n0.net

NEWS.adoc

@@ -73,6 +73,14 @@ https://github.com/networkupstools/nut/milestone/9
      and misfired on some platforms, and the way to print had a theoretical
      potential for buffer overflow. [#2915]
 
+ - common driver code:
+   * Removed workarounds trying to migrate legacy driver raised `ALARM`
+     status tokens into modern `alarm_*` function logic. Rather, we keep
+     supporting them as separate from the modern logic, seeing as `upsmon`
+     does not care where the token itself was raised for its notifications.
+     Driver-code related test-cases were updated to reflect these changes.
+     [issue #2928, PRs #2931 and #2934]
+
  - `nutdrv_qx` driver updates:
    * Introduced `innovart33` protocol support for Ippon Innova RT 3/3 topology
      UPSes. [#2938]

clients/upsmon.c

@@ -820,7 +820,8 @@ static void ups_is_alarm(utype_t *ups)
 				do_notify(ups, NOTIFY_ALARM, alarms);
 			}
 		} else {
-			upsdebugx(4, "%s: %s: still on alarm, failed to get current ups.alarm value",
+			upsdebugx(4, "%s: %s: still on alarm, failed to get current ups.alarm value, "
+				"perhaps it is just a legacy driver (but we keep probing it to be safe)",
 				__func__, ups->sys);
 		}
 		return;
@@ -842,9 +843,10 @@ static void ups_is_alarm(utype_t *ups)
 			__func__, ups->sys, alarms);
 		do_notify(ups, NOTIFY_ALARM, alarms);
 	} else {
-		upsdebugx(4, "%s: %s: failed to get current ups.alarm value",
+		upsdebugx(4, "%s: %s: failed to get current ups.alarm value, perhaps "
+			"it is just a legacy driver (but we keep probing it to be safe)",
 			__func__, ups->sys);
-		do_notify(ups, NOTIFY_ALARM, NULL);
+		do_notify(ups, NOTIFY_ALARM, "n/a");
 		alarms[0] = '\0';
 	}
 	strncpy(alarms_prev, alarms, sizeof(alarms_prev));

docs/developers.txt

@@ -202,7 +202,7 @@ function avoid_vla(int len)
 }
 --------------------------------------------------------------------------------
 
-This should be sued instead (preferably with `size_t` arguments right away),
+This should be used instead (preferably with `size_t` arguments right away),
 be sure to `free()` anywhere you `return` from the function:
 --------------------------------------------------------------------------------
 int use_xcalloc(size_t len)

docs/new-drivers.txt

@@ -218,6 +218,10 @@ formatting string to those expectations.  In this case, you would use
 	char *fmt = "Mega-Zapper %d";
 	dstate_setinfo_dynamic("ups.model", fmt, "%d", rating);
 
+Please note that `ups.alarm` should no longer be manually set, but rather
+the appropriate alarm functions should be used instead. For more details,
+see below in the `UPS alarms` section.
+
 Setting flags
 ~~~~~~~~~~~~~
 
@@ -261,9 +265,10 @@ Possible values for status_set:
 	BOOST   -- UPS is boosting incoming voltage
 	FSD     -- Forced Shutdown (restricted use, see the note below)
 
-Internally, an `ALARM` value would be added (typically as first in the list)
-if the `ups.alarm` is currently not empty. For more details, see below in
-`alarm_set()` description.
+`ALARM` should no longer be raised through the UPS status.
+An `ALARM` value will be added internally (typically as first in the list)
+if an alarm was set and committed through the appropriate alarm functions.
+For more details, see below in the `UPS alarms` section.
 
 [NOTE]
 ======

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3480 utf-8
+personal_ws-1.1 en 3493 utf-8
 AAC
 AAS
 ABI
@@ -232,9 +232,11 @@ CygWin
 Cygwin
 DATACABLE
 DATAPATH
+DBCF
 DCE
 DCF
 DCO
+DDA
 DDD
 DDDDD
 DDDDDD
@@ -369,7 +371,9 @@ Exar
 ExecCGI
 ExecStart
 ExecStartPre
+FBCA
 FD
+FDE
 FEMEA
 FFF
 FH
@@ -1029,6 +1033,7 @@ QLDL
 QMD
 QMF
 QMOD
+QOL
 QP
 QPAR
 QPD
@@ -1893,6 +1898,7 @@ desc
 deschis
 descr
 desde
+desertwitch
 designator
 dev
 devclient

drivers/dstate.c

@@ -5,6 +5,7 @@
 	2008		Arjen de Korte <[email protected]>
 	2012-2017	Arnaud Quette <[email protected]>
 	2020-2025	Jim Klimov <[email protected]>
+	2025		desertwitch <[email protected]>
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -50,7 +51,8 @@
 	static OVERLAPPED	connect_overlapped;
 	static char	*pipename = NULL;
 #endif	/* WIN32 */
-	static int	stale = 1, alarm_active = 0, alarm_status = 0, ignorelb = 0;
+	static int	stale = 1, alarm_active = 0, alarm_status = 0, ignorelb = 0,
+				alarm_legacy_status = 0;
 	static char	status_buf[ST_MAX_VALUE_LEN], alarm_buf[ST_MAX_VALUE_LEN],
 			buzzmode_buf[ST_MAX_VALUE_LEN];
 	static conn_t	*connhead = NULL;
@@ -1766,12 +1768,12 @@ int dstate_is_stale(void)
 /* clean out the temp space for a new pass */
 void status_init(void)
 {
-	if (dstate_getinfo("driver.flag.ignorelb")) {
-		ignorelb = 1;
-	}
+	/* This does not normally change in driver run-time, but can in tests */
+	ignorelb = (dstate_getinfo("driver.flag.ignorelb") ? 1 : 0);
 
 	memset(status_buf, 0, sizeof(status_buf));
 	alarm_status = 0;
+	alarm_legacy_status = 0;
 }
 
 /* check if a status element has been set, return 0 if not, 1 if yes
@@ -1795,17 +1797,23 @@ static int status_set_callback(char *tgt, size_t tgtsize, const char *token)
 	}
 
 	if (!strcasecmp(token, "ALARM")) {
-		/* Drivers really should not raise alarms this way,
-		 * but for the sake of third-party forks, we handle
-		 * the possibility...
+		/* Drivers should not do this anymore, but we continue
+		 * to support it. The upsmon notifiers do not care where
+		 * alarm tokens get set and legacy drivers may still use
+		 * this older method. The only real limitations are that
+		 * the alarm state is very tightly coupled to the UPS
+		 * status, and the ups.alarm variables do not get published
+		 * in a controlled manner. Rather, these are very dependent
+		 * on the (legacy) driver, and alarm notifications via upsmon
+		 * will show placeholder "n/a" if all values are missing.
+		 * The status token is ignored to prepend it to other tokens
+		 * later, within the status_commit() function.
+		 * For more information, see this discussion:
+		 * https://github.com/networkupstools/nut/pull/2931#issuecomment-2841705269
 		 */
-		upsdebugx(2, "%s: (almost) ignoring ALARM set as a status", __func__);
-		if (!alarm_status && !alarm_active && strlen(alarm_buf) == 0) {
-			alarm_init();	/* no-op currently, but better be proper about it */
-			alarm_set("[N/A]");
-		}
-		alarm_status++;
-		return 0;
+		upsdebugx(6, "%s: caller set ALARM as a status, this is deprecated - please fix the NUT driver code", __func__);
+		alarm_legacy_status = 1;
+		return 0; /* ignore it */
 	}
 
 	/* Proceed adding the token */
@@ -1848,36 +1856,15 @@ void status_commit(void)
 		break;
 	}
 
-	/* NOTE: Not sure if any clients rely on ALARM being first if raised,
-	 * but note that if someone also uses status_set("ALARM") we can end
-	 * up with a "[N/A]" alarm value injected (if no other alarm was set)
-	 * and only add the token here so it remains first.
-	 *
-	 * NOTE: alarm_commit() must be executed before status_commit() for
-	 * this report to work!
-	 * * If a driver only called status_set("ALARM") and did not bother
-	 *   with alarm_commit(), the "ups.alarm" value queries would have
-	 *   returned NULL if not for the "sloppy driver" fix below, although
-	 *   the "ups.status" value would report an ALARM token.
-	 * * If a driver properly used alarm_init() and alarm_set(), but then
-	 *   called status_commit() before alarm_commit(), the "ups.status"
-	 *   value would not know to report an ALARM token, as before.
-	 * * If a driver used both status_set("ALARM") and alarm_set() later,
-	 *   the injected "[N/A]" value of the alarm (if that's its complete
-	 *   value) would be overwritten by the explicitly assigned contents,
-	 *   and an explicit alarm_commit() would be required for proper
-	 *   reporting from a non-sloppy driver.
-	 */
-
-	if (!alarm_active && alarm_status && !strcmp(alarm_buf, "[N/A]")) {
-		upsdebugx(2, "%s: Assume sloppy driver coding that ignored alarm methods and used status_set(\"ALARM\") instead: commit the injected N/A ups.alarm value", __func__);
-		alarm_commit();
-	}
-
-	if (alarm_active) {
-		dstate_setinfo("ups.status", "ALARM %s", status_buf);
+	if (alarm_active || alarm_legacy_status) {
+		if (*status_buf != '\0') {
+			dstate_setinfo("ups.status", "ALARM %s", status_buf);
+		} else {
+			dstate_setinfo("ups.status", "ALARM");
+		}
 	} else {
 		dstate_setinfo("ups.status", "%s", status_buf);
+		alarm_legacy_status = 0; /* just to be sure */
 	}
 }