Merge pull request #2383 from jimklimov/issue-321

Clarify the value of POWERDOWNFLAG in `upsmon -h` and document that it must be set

NEWS.adoc

@@ -40,6 +40,15 @@ https://github.com/networkupstools/nut/milestone/11
  - (expected) Bug fixes for fallout possible due to "fightwarn" effort in 2.8.0
 
 
+
+ - upsmon: it was realized that the `POWERDOWNFLAG` must be explicitly set in
+   the configuration file, there is no built-in default in the binary program
+   (the settings facilitated by the `configure` script during build "only"
+   impact the `upsmon.conf.sample`, init-scripts and similar files generated
+   from templates). [issue #321, PR #2383]
+
+
+
 Release notes for NUT 2.8.2 - what's new since 2.8.1
 ----------------------------------------------------
 

clients/upsmon.c

@@ -1492,22 +1492,28 @@ static void addups(int reloading, const char *sys, const char *pvs,
 	else
 		firstups = tmp;
 
-	if (tmp->pv)
-		upslogx(LOG_INFO, "UPS: %s (%s) (power value %d)", tmp->sys,
-			flag_isset(tmp->status, ST_PRIMARY) ? "primary" : "secondary",
-			tmp->pv);
-	else
-		upslogx(LOG_INFO, "UPS: %s (monitoring only)", tmp->sys);
+	/* Negative debug value may be set by help() to be really quiet */
+	if (nut_debug_level > -1) {
+		if (tmp->pv)
+			upslogx(LOG_INFO, "UPS: %s (%s) (power value %d)", tmp->sys,
+				flag_isset(tmp->status, ST_PRIMARY) ? "primary" : "secondary",
+				tmp->pv);
+		else
+			upslogx(LOG_INFO, "UPS: %s (monitoring only)", tmp->sys);
+	}
 
 	tmp->upsname = tmp->hostname = NULL;
 
 	if (upscli_splitname(tmp->sys, &tmp->upsname, &tmp->hostname,
-		&tmp->port) != 0) {
-		upslogx(LOG_ERR, "Error: unable to split UPS name [%s]",
-			tmp->sys);
+		&tmp->port) != 0
+	) {
+		if (nut_debug_level > -1) {
+			upslogx(LOG_ERR, "Error: unable to split UPS name [%s]",
+				tmp->sys);
+		}
 	}
 
-	if (!tmp->upsname)
+	if (!tmp->upsname && nut_debug_level > -1)
 		upslogx(LOG_WARNING, "Warning: UPS [%s]: no upsname set!",
 			tmp->sys);
 }
@@ -1587,8 +1593,10 @@ static void checkmode(char *cfgentry, char *oldvalue, char *newvalue,
 	if (use_pipe == 0)
 		return;
 
-	/* it's ok if we're not reloading yet */
-	if (reloading == 0)
+	/* it's ok if we're not reloading yet
+	 * or "almost-fake" loading to show help()
+	 */
+	if (reloading < 1)
 		return;
 
 	/* also nothing to do if it didn't change */
@@ -1652,7 +1660,7 @@ static int parse_conf_arg(size_t numargs, char **arg)
 		powerdownflag = filter_path(arg[1]);
 #endif
 
-		if (!reload_flag)
+		if (reload_flag == 0)
 			upslogx(LOG_INFO, "Using power down flag file %s",
 				arg[1]);
 
@@ -1854,6 +1862,10 @@ static void loadconfig(void)
 	PCONF_CTX_t	ctx;
 	int	numerrors = 0;
 
+	upsdebugx(1, "%s: %s %s", __func__,
+		(reload_flag == 1) ? "Reloading" : "Loading",
+		configfile);
+
 	pconf_init(&ctx, upsmon_err);
 
 	if (!pconf_file_begin(&ctx, configfile)) {
@@ -1864,6 +1876,12 @@ static void loadconfig(void)
 			return;
 		}
 
+		if (reload_flag == -1) {
+			/* For help() */
+			upsdebugx(1, "Load failed: %s", ctx.errmsg);
+			return;
+		}
+
 		fatalx(EXIT_FAILURE, "%s", ctx.errmsg);
 	}
 
@@ -1930,11 +1948,14 @@ static void loadconfig(void)
 				nut_debug_level_global);
 			nut_debug_level = nut_debug_level_global;
 		} else {
-			/* DEBUG_MIN is absent or commented-away in ups.conf */
-			upslogx(LOG_INFO,
-				"Applying debug level %d from "
-				"original command line arguments",
-				nut_debug_level_args);
+			/* DEBUG_MIN is absent or commented-away in ups.conf
+			 * Negative value may be set by help() to be really quiet
+			 */
+			if (nut_debug_level_args > -1)
+				upslogx(LOG_INFO,
+					"Applying debug level %d from "
+					"original command line arguments",
+					nut_debug_level_args);
 			nut_debug_level = nut_debug_level_args;
 		}
 
@@ -1953,6 +1974,16 @@ static void loadconfig(void)
 	}
 
 	pconf_finish(&ctx);
+
+	/* TOTHINK: Should this warning be limited to non-WIN32 builds? */
+	if (!powerdownflag) {
+		upslogx(LOG_WARNING, "No POWERDOWNFLAG value was configured in %s!",
+			configfile);
+		upslogx(LOG_INFO,
+			"Should be a path to file that is normally writeable "
+			"for root user, and remains at least readable late "
+			"in shutdown after all unmounting completes.");
+	}
 }
 
 #ifndef WIN32
@@ -2445,6 +2476,30 @@ static void help(const char *arg_progname)
 
 static void help(const char *arg_progname)
 {
+	int old_debug_level = nut_debug_level;
+	int old_debug_level_args = nut_debug_level_args;
+	int old_debug_level_global = nut_debug_level_global;
+
+	/* Try to get POWERDOWNFLAG? */
+	if (!powerdownflag) {
+		/* Avoid fatalx() on bad/absent configs */
+		reload_flag = -1;
+
+		/* Hush messages normally emitted by loadconfig() */
+		nut_debug_level = -2;
+		nut_debug_level_args = -2;
+		nut_debug_level_global = -2;
+
+		loadconfig();
+
+		nut_debug_level = old_debug_level;
+		nut_debug_level_args = old_debug_level_args;
+		nut_debug_level_global = old_debug_level_global;
+
+		/* Separate from logs emitted by loadconfig() */
+		/* printf("\n"); */
+	}
+
 	printf("Monitors UPS servers and may initiate shutdown if necessary.\n\n");
 
 	printf("usage: %s [OPTIONS]\n\n", arg_progname);
@@ -2461,7 +2516,8 @@ static void help(const char *arg_progname)
 	printf("  -B		stay backgrounded even if debugging is bumped\n");
 	printf("  -V		display the version of this software\n");
 	printf("  -h		display this help\n");
-	printf("  -K		checks POWERDOWNFLAG, sets exit code to 0 if set\n");
+	printf("  -K		checks POWERDOWNFLAG (%s), sets exit code to 0 if set\n",
+		powerdownflag ? powerdownflag : "***NOT CONFIGURED***");
 	printf("  -p		always run privileged (disable privileged parent)\n");
 	printf("  -u <user>	run child as user <user> (ignored when using -p)\n");
 	printf("  -4		IPv4 only\n");

conf/upsmon.conf.sample.in

@@ -1,6 +1,11 @@
 # Network UPS Tools: example upsmon configuration
 #
 # This file contains passwords, so keep it secure.
+#
+# A minimal configuration should include at least one MONITOR instruction,
+# MINSUPPLIES (may be 0 if this system is only monitoring other NUT servers),
+# and a POWERDOWNFLAG if this machine is a "primary" system connected to
+# the UPS and drives its late-shutdown power-off command in an emergency.
 
 # --------------------------------------------------------------------------
 # RUN_AS_USER <userid>
@@ -292,8 +297,13 @@ DEADTIME 15
 # For Windows setup use something like:
 # POWERDOWNFLAG "C:\\killpower"
 #
-# For Unix/Linux systems the legacy default is:
-# POWERDOWNFLAG /etc/killpower
+# For Unix/Linux systems the legacy common value is:
+#   POWERDOWNFLAG /etc/killpower
+# but nowadays you may want it in a temporary filesystem (e.g. under /run)
+#
+# WARNING: The 'upsmon' binary program does not have a built-in default,
+# so this setting MUST be specified in the configuration, in order for the
+# late shutdown integration to work on the particular primary-mode system!
 POWERDOWNFLAG "@POWERDOWNFLAG@"
 
 # --------------------------------------------------------------------------

docs/man/upsmon.conf.txt

@@ -14,6 +14,11 @@ will monitor and to tell it how to shut down the system when necessary.
 It will contain passwords, so keep it secure.  Ideally, only the upsmon
 process should be able to read it.
 
+A minimal configuration should include at least one `MONITOR` instruction,
+`MINSUPPLIES` (may be 0 if this system is only monitoring other NUT servers),
+and a `POWERDOWNFLAG` if this machine is a "primary" system connected to
+the UPS and drives its late-shutdown power-off command in an emergency.
+
 Additionally, other optional configuration values can be set in this
 file.
 
@@ -319,14 +324,27 @@ also apply here.
 *POWERDOWNFLAG* 'filename'::
 
 upsmon creates this file when running in primary mode when the UPS needs
-to be powered off.  You should check for this file in your shutdown
-scripts and call `upsdrvctl shutdown` if it exists.
+to be powered off.  You should check for this file in your late shutdown
+scripts and call `upsdrvctl shutdown` if it exists; note that `upsmon -K`
+may be called for this effect, if NUT configuration files remain readable
+at that point (file systems mostly unmounted or changed to read-only).
++
+Historically it was often `/etc/killpower` but nowadays you may want it
+in a temporary filesystem (e.g. under `/run` or `/run/nut` location).
++
+Note that double backslashes must be used for Windows paths, e.g.
+`C:\\Temp\\killpower` (modern Windows may also accept forward slashes
+like `C:/Temp/killpower` but YMMV).
 +
 This is done to forcibly reset the secondary systems, so they don't get
 stuck at the "halted" stage even if the power returns during the shutdown
 process.  This usually does not work well on contact-closure UPSes that
 use the genericups driver.
 +
+WARNING: The `upsmon` binary program does not have a built-in default,
+so this setting MUST be specified in the configuration, in order for the
+late shutdown integration to work on the particular primary-mode system!
++
 See the config-notes.txt file in the docs subdirectory for more information.
 Refer to the section:
 [[UPS_shutdown]] "Configuring automatic shutdowns for low battery events",