Merge pull request networkupstools#2686 from jimklimov/issue-2670

Introduce `sdcommands` option and `shutdown.default` INSTCMD to all drivers

NEWS.adoc

@@ -71,6 +71,12 @@ https://github.com/networkupstools/nut/milestone/11
      a `*_s()` variant was available was not handled correctly. [PR #2583]
    * A recently introduced `allow_killpower` did not actually work as an
      `ups.conf` flag (only as a protocol command). [issue #2605, PR #2606]
+   * The ability of two copies of the driver program to talk to each other
+     with `upsdrvquery.c` code was not complete for the case of indefinite
+     `select()` wait timeout. Now `upsdrvquery_read_timeout()` fixed private
+     use of `struct timeval={-1,-1}` as a trigger to `select(..., NULL)`,
+     as logged in one part of code and not handled in the other, for the
+     indefinite wait [#1922, #2392, #2686, #2670]
 
  - development iterations of NUT should now identify with not only the semantic
    version of a preceding release, but with git-derived information about the
@@ -339,6 +345,24 @@ during a NUT build.
      for `bcmxcp_usb`, `richcomm_usb` and `nutdrv_atcl_usb` drivers for now
      [#1763, #1764, #1768, #2580]
 
+ - all drivers should now support the optional `sdcommands` setting with
+   a site-local list of instant commands to handle `upsdrv_shutdown()`,
+   which may be useful in cases when the driver's built-in commands
+   (or their order) do not meet the goals of particular NUT deployment.
+   This can also help with shutdown endgame testing, using a mock command like
+   starting the beeper (where supported) to verify that the UPS communications
+   happen as expected, without compromising the load connected to the UPS.
++
+Also defined `EF_EXIT_SUCCESS` and `EF_EXIT_FAILURE` in `include/common.h`
+to avoid magic numbers in code like `set_exit_flag(-2)`, and revised whether
+it is getting set at all in "killpower" vs. other cases, based on new
+`handling_upsdrv_shutdown` internal flag.
++
+NOTE: during this overhaul, many older drivers got their first ever supported
+INSTCMD such as `shutdown.return`, `shutdown.stayoff` or `load.off`. Default
+logic that was previously the content of `upsdrv_shutdown()` methods was often
+relocated into new `shutdown.default` INSTCMD definitions. [#2670]
+
  - common code:
    * introduced a `NUT_DEBUG_SYSLOG` environment variable to tweak activation
      of syslog message emission (and related detachment of `stderr` when

clients/upscmd.c

@@ -195,6 +195,9 @@ static void do_cmd(char **argv, const int argc)
 	) {
 		/* reply as usual */
 		fprintf(stderr, "%s\n", buf);
+		upsdebugx(1, "%s: 'OK' only means the NUT data server accepted the request as valid, "
+			"but as we did not wait for result, we do not know if it was handled in fact.",
+			__func__);
 		return;
 	}
 
@@ -282,9 +285,20 @@ int main(int argc, char **argv)
 	uint16_t	port;
 	ssize_t	ret;
 	int	have_un = 0, have_pw = 0, cmdlist = 0;
-	char	buf[SMALLBUF * 2], username[SMALLBUF], password[SMALLBUF];
+	char	buf[SMALLBUF * 2], username[SMALLBUF], password[SMALLBUF], *s = NULL;
 	const char	*prog = xbasename(argv[0]);
 
+	/* NOTE: Caller must `export NUT_DEBUG_LEVEL` to see debugs for upsc
+	 * and NUT methods called from it. This line aims to just initialize
+	 * the subsystem, and set initial timestamp. Debugging the client is
+	 * primarily of use to developers, so is not exposed via `-D` args.
+	 */
+	s = getenv("NUT_DEBUG_LEVEL");
+	if (s && str_to_int(s, &i, 10) && i > 0) {
+		nut_debug_level = i;
+	}
+	upsdebugx(1, "Starting NUT client: %s", prog);
+
 	while ((i = getopt(argc, argv, "+lhu:p:t:wV")) != -1) {
 
 		switch (i)
@@ -459,6 +473,6 @@ int main(int argc, char **argv)
 /* Formal do_upsconf_args implementation to satisfy linker on AIX */
 #if (defined NUT_PLATFORM_AIX)
 void do_upsconf_args(char *upsname, char *var, char *val) {
-        fatalx(EXIT_FAILURE, "INTERNAL ERROR: formal do_upsconf_args called");
+	fatalx(EXIT_FAILURE, "INTERNAL ERROR: formal do_upsconf_args called");
 }
 #endif  /* end of #if (defined NUT_PLATFORM_AIX) */

clients/upsrw.c

@@ -121,6 +121,9 @@ static void do_set(const char *varname, const char *newval)
 	) {
 		/* reply as usual */
 		fprintf(stderr, "%s\n", buf);
+		upsdebugx(1, "%s: 'OK' only means the NUT data server accepted the request as valid, "
+			"but as we did not wait for result, we do not know if it was handled in fact.",
+			__func__);
 		return;
 	}
 
@@ -641,7 +644,18 @@ int main(int argc, char **argv)
 	int	i;
 	uint16_t	port;
 	const char	*prog = xbasename(argv[0]);
-	char	*password = NULL, *username = NULL, *setvar = NULL;
+	char	*password = NULL, *username = NULL, *setvar = NULL, *s = NULL;
+
+	/* NOTE: Caller must `export NUT_DEBUG_LEVEL` to see debugs for upsc
+	 * and NUT methods called from it. This line aims to just initialize
+	 * the subsystem, and set initial timestamp. Debugging the client is
+	 * primarily of use to developers, so is not exposed via `-D` args.
+	 */
+	s = getenv("NUT_DEBUG_LEVEL");
+	if (s && str_to_int(s, &i, 10) && i > 0) {
+		nut_debug_level = i;
+	}
+	upsdebugx(1, "Starting NUT client: %s", prog);
 
 	while ((i = getopt(argc, argv, "+hls:p:t:u:wV")) != -1) {
 		switch (i)
@@ -717,6 +731,6 @@ int main(int argc, char **argv)
 /* Formal do_upsconf_args implementation to satisfy linker on AIX */
 #if (defined NUT_PLATFORM_AIX)
 void do_upsconf_args(char *upsname, char *var, char *val) {
-        fatalx(EXIT_FAILURE, "INTERNAL ERROR: formal do_upsconf_args called");
+	fatalx(EXIT_FAILURE, "INTERNAL ERROR: formal do_upsconf_args called");
 }
 #endif  /* end of #if (defined NUT_PLATFORM_AIX) */

conf/ups.conf.sample

@@ -160,6 +160,12 @@ maxretry = 3
 #
 #          The default value for this parameter is 0.
 #
+# sdcommands: OPTIONAL.  Comma-separated list of instant command name(s)
+#          to send to the UPS when you request its shutdown.  For more
+#          details about relevant use-cases see the ups.conf manual page.
+#
+#          The default value is built into each driver (where supported).
+#
 #    desc: optional, to keep a note of the UPS purpose, location, etc.
 #
 #  nolock: optional, and not recommended for use in this file.

data/cmdvartab

@@ -221,6 +221,7 @@ CMDDESC driver.reload-or-exit "Reload running driver configuration from the file
 
 CMDDESC load.off "Turn off the load immediately"
 CMDDESC load.on "Turn on the load immediately"
+CMDDESC shutdown.default "Run the driver-defined UPS shutdown sequence (as opposed to user-configured 'sdcommands')"
 CMDDESC shutdown.return "Turn off the load and return when power is back"
 CMDDESC shutdown.stayoff "Turn off the load and remain off"
 CMDDESC shutdown.stop "Stop a shutdown in progress"

docs/man/ups.conf.txt

@@ -215,6 +215,31 @@ set this to -1.
 +
 The default value for this parameter is 0.
 
+*sdcommands*::
+
+Optional.  Comma-separated list of instant command name(s) to send to
+the UPS when you request its shutdown.
++
+Default logic is built into each driver (where supported) and can be
+referenced here as the `shutdown.default` value.
++
+The primary use-case is for devices whose drivers "natively" support
+trying several commands, but the built-in order of those calls a
+command that is mis-handled by the specific device model (so the
+handling is reported as successful and the loop stops, but nothing
+happens as far as the load power-down is concerned).
++
+Another use-case is differentiation of automated power-off scenarios
+where the UPS and its load should stay "OFF" (e.g. by building emergency
+power-off) vs. those where the load should return to work automatically
+when it is safe to do so.  NOTE: This would *currently* need editing of
+`ups.conf` for such cases before `nutshutdown` sees the file; but could
+be better automated in future NUT releases.
++
+NOTE: User-provided commands may be something other than actual shutdown,
+e.g. a beeper to test that the INSTCMD happened such and when expected,
+and the device was contacted, without impacting the load fed by the UPS.
+
 *allow_killpower*::
 Optional.  This allows you to request `driver.killpower` instant command,
 to immediately call the driver-specific default implementation of

docs/new-drivers.txt

@@ -165,9 +165,10 @@ process.
 This method should not directly `exit()` the driver program (neither
 should it call `fatalx()` nor `fatal_with_errno()` methods).  It can
 `upslogx(LOG_ERR, ...)` or `upslog_with_errno(LOG_ERR, ...)`, and then
-`set_exit_flag(N)` if required (`-1` for `EXIT_FAILURE` and `-2` for
-`EXIT_SUCCESS` which would be handled in the standard driver loop or
-`forceshutdown()` method of `main.c`).
+`set_exit_flag(N)` if required, using values `EF_EXIT_FAILURE` (`-1`)
+for eventual `exit(EXIT_FAILURE)` and `EF_EXIT_SUCCESS` (`-2`) for
+`exit(EXIT_SUCCESS)`, which would be handled in the standard driver
+loop or in `forceshutdown()` method of `main.c`.
 
 Data types
 ----------

docs/nut-names.txt

@@ -861,6 +861,13 @@ Instant commands
 | load.on                  | Turn on the load immediately
 | load.off.delay           | Turn off the load possibly after a delay
 | load.on.delay            | Turn on the load possibly after a delay
+| shutdown.default         | Run default driver-defined (device-specific)
+                             routine, primarily intended for emergency
+                             poweroff performed as part of FSD handling;
+                             often an alias to other `shutdown.*` and/or
+                             `load.off` operations or a chain to try
+                             several of those. See also `sdcommands` in
+                             common driver options.
 | shutdown.return          | Turn off the load possibly after a delay
                              and return when power is back
 | shutdown.stayoff         | Turn off the load possibly after a delay

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3260 utf-8
+personal_ws-1.1 en 3263 utf-8
 AAC
 AAS
 ABI
@@ -2765,6 +2765,7 @@ screenshots
 scriptname
 sd
 sdcmd
+sdcommands
 sddelay
 sdk
 sdl
@@ -2989,6 +2990,7 @@ timehead
 timername
 timestamp
 timeticks
+timeval
 tios
 tmp
 tmpfiles
@@ -3087,6 +3089,7 @@ upsdebugx
 upsdev
 upsdrv
 upsdrvctl
+upsdrvquery
 upsdrvsvcctl
 upserror
 upsfetch

drivers/adelsystem_cbi.c

@@ -30,7 +30,7 @@
 #include <timehead.h>
 
 #define DRIVER_NAME "NUT ADELSYSTEM DC-UPS CB/CBI driver"
-#define DRIVER_VERSION "0.03"
+#define DRIVER_VERSION "0.04"
 
 /* variables */
 static modbus_t *mbctx = NULL;							/* modbus memory context */
@@ -49,7 +49,6 @@ static uint32_t mod_resp_to_us = MODRESP_TIMEOUT_us;	/* set the modbus response
 static uint32_t mod_byte_to_s = MODBYTE_TIMEOUT_s;		/* set the modbus byte time out (us) */
 static uint32_t mod_byte_to_us = MODBYTE_TIMEOUT_us;	/* set the modbus byte time out (us) */
 
-
 /* initialize alarm structs */
 void alrminit(void);
 
@@ -193,6 +192,11 @@ void upsdrv_initinfo(void)
 	/* register instant commands */
 	dstate_addcmd("load.off");
 
+	/* FIXME: Check with the device what this instcmd
+	 * (nee upsdrv_shutdown() contents) actually does!
+	 */
+	dstate_addcmd("shutdown.stayoff");
+
 	/* set callback for instant commands */
 	upsh.instcmd = upscmd;
 }
@@ -464,33 +468,24 @@ void upsdrv_updateinfo(void)
 /* shutdown UPS */
 void upsdrv_shutdown(void)
 {
-	int rval;
-	int cnt = FSD_REPEAT_CNT;	 /* shutdown repeat counter */
-	struct timeval start;
-	long etime;
-
-	/* retry sending shutdown command on error */
-	while ((rval = upscmd("load.off", NULL)) != STAT_INSTCMD_HANDLED && cnt > 0) {
-		rval = gettimeofday(&start, NULL);
-		if (rval < 0) {
-			upslog_with_errno(LOG_ERR, "upscmd: gettimeofday");
-		}
+	/* Only implement "shutdown.default"; do not invoke
+	 * general handling of other `sdcommands` here */
 
-		/* wait for an increasing time interval before sending shutdown command */
-		while ((etime = time_elapsed(&start)) < ( FSD_REPEAT_INTRV / cnt));
-		upsdebugx(2, "ERROR: load.off failed, wait for %lims, retries left: %d\n", etime, cnt - 1);
-		cnt--;
-	}
-	switch (rval) {
-		case STAT_INSTCMD_FAILED:
-		case STAT_INSTCMD_INVALID:
-			fatalx(EXIT_FAILURE, "shutdown failed");
-		case STAT_INSTCMD_UNKNOWN:
-			fatalx(EXIT_FAILURE, "shutdown not supported");
-		default:
-			break;
-	}
-	upslogx(LOG_INFO, "shutdown command executed");
+	/*
+	 * WARNING: When using RTU TCP, this driver will probably
+	 * never support shutdowns properly, except on some systems:
+	 * In order to be of any use, the driver should be called
+	 * near the end of the system halt script (or a service
+	 * management framework's equivalent, if any). By that
+	 * time we, in all likelyhood, won't have basic network
+	 * capabilities anymore, so we could never send this
+	 * command to the UPS. This is not an error, but rather
+	 * a limitation (on some platforms) of the interface/media
+	 * used for these devices.
+	 */
+	int	ret = do_loop_shutdown_commands("shutdown.stayoff", NULL);
+	if (handling_upsdrv_shutdown > 0)
+		set_exit_flag(ret == STAT_INSTCMD_HANDLED ? EF_EXIT_SUCCESS : EF_EXIT_FAILURE);
 }
 
 /* print driver usage info */
@@ -832,6 +827,43 @@ int upscmd(const char *cmd, const char *arg)
 			upsdebugx(2, "load.off: addr: 0x%x, data: %d", regs[FSD].xaddr, data);
 			rval = STAT_INSTCMD_HANDLED;
 		}
+	} else if (!strcasecmp(cmd, "shutdown.stayoff")) {
+		/* FIXME: Which one is this actually -
+		 * "shutdown.stayoff" or "shutdown.return"? */
+		int cnt = FSD_REPEAT_CNT;	 /* shutdown repeat counter */
+		struct timeval start;
+		long etime;
+
+		/* retry sending shutdown command on error */
+		while ((rval = upscmd("load.off", NULL)) != STAT_INSTCMD_HANDLED && cnt > 0) {
+			rval = gettimeofday(&start, NULL);
+			if (rval < 0) {
+				upslog_with_errno(LOG_ERR, "upscmd: gettimeofday");
+			}
+
+			/* wait for an increasing time interval before sending shutdown command */
+			while ((etime = time_elapsed(&start)) < ( FSD_REPEAT_INTRV / cnt));
+			upsdebugx(2, "ERROR: load.off failed, wait for %lims, retries left: %d\n", etime, cnt - 1);
+			cnt--;
+		}
+		switch (rval) {
+			case STAT_INSTCMD_FAILED:
+			case STAT_INSTCMD_INVALID:
+				upslog_with_errno(LOG_ERR, "instcmd: %s failed", cmd);
+				if (handling_upsdrv_shutdown > 0)
+					set_exit_flag(EF_EXIT_FAILURE);
+				break;
+			case STAT_INSTCMD_UNKNOWN:
+				upslog_with_errno(LOG_ERR, "instcmd: %s not supported", cmd);
+				if (handling_upsdrv_shutdown > 0)
+					set_exit_flag(EF_EXIT_FAILURE);
+				break;
+			default:
+				upslogx(LOG_INFO, "shutdown command executed");
+				if (handling_upsdrv_shutdown > 0)
+					set_exit_flag(EF_EXIT_SUCCESS);
+				break;
+		}
 	} else {
 		upslogx(LOG_NOTICE, "instcmd: unknown command [%s] [%s]", cmd, arg);
 		rval = STAT_INSTCMD_UNKNOWN;

drivers/al175.c

@@ -52,7 +52,7 @@ typedef	uint8_t byte_t;
 
 
 #define DRIVER_NAME	"Eltek AL175/COMLI driver"
-#define DRIVER_VERSION	"0.15"
+#define DRIVER_VERSION	"0.16"
 
 /* driver description structure */
 upsdrv_info_t upsdrv_info = {
@@ -1267,6 +1267,9 @@ void upsdrv_updateinfo(void)
 
 void upsdrv_shutdown(void)
 {
+	/* Only implement "shutdown.default"; do not invoke
+	 * general handling of other `sdcommands` here */
+
 	/* TODO use TOGGLE_PRS_ONOFF for shutdown */
 
 	/* tell the UPS to shut down, then return - DO NOT SLEEP HERE */
@@ -1276,7 +1279,8 @@ void upsdrv_shutdown(void)
 
 	/* replace with a proper shutdown function */
 	upslogx(LOG_ERR, "shutdown not supported");
-	set_exit_flag(-1);
+	if (handling_upsdrv_shutdown > 0)
+		set_exit_flag(EF_EXIT_FAILURE);
 
 	/* you may have to check the line status since the commands
 	   for toggling power are frequently different for OL vs. OB */