Merge pull request networkupstools#2572 from jimklimov/issue-2568-win

Follow-up to `upsdrvctl status` on Windows, add `LOGOUT` to local socket protocol

NEWS.adoc

@@ -217,6 +217,8 @@ during a NUT build.
      and values (or macros) in the NUT codebase. [issue #1176, issue #31]
    * custom `distcheck-something` targets did not inherit `DISTCHECK_FLAGS`
      properly. [#2541]
+   * local socket/pipe protocol introduced a `LOGOUT` command for cleaner
+     disconnection handling. [#2572]
 
  - updated `docs/nut-names.txt` with items defined by 42ITy NUT fork. [#2339]
 

common/common.c

@@ -1412,8 +1412,10 @@ pid_t parsepid(const char *buf)
 	pid_t	pid = -1;
 	intmax_t	_pid;
 
+	errno = 0;
 	if (!buf) {
 		upsdebugx(6, "%s: called with NULL input", __func__);
+		errno = EINVAL;
 		return pid;
 	}
 
@@ -1422,6 +1424,8 @@ pid_t parsepid(const char *buf)
 	if (_pid <= get_max_pid_t()) {
 		pid = (pid_t)_pid;
 	} else {
+		errno = ERANGE;
+
 		if (nut_debug_level > 0 || nut_sendsignal_debug_level > 0)
 			upslogx(LOG_NOTICE,
 				"Received a pid number too big for a pid_t: %"

docs/man/upsdrvctl.txt

@@ -134,7 +134,12 @@ here and managed in other commands).
 
 NOTE: The tool name and NUT version banner line is also printed to `stdout`
 before any other processing. This can be suppressed by `NUT_QUIET_INIT_BANNER`
-environment variable (exported by caller and empty or "true").
+environment variable (exported by caller and empty or "true"):
+
+	:; NUT_QUIET_INIT_BANNER=true upsdrvctl list
+	dummy
+	UPS1
+	UPS2
 
 *status*::
 Similar to `list`, but reports more information -- also the driver name, the
@@ -144,30 +149,38 @@ If there is anything to print (at least one device is known), the first line
 of status report would be the heading with column names:
 
 	:; NUT_QUIET_INIT_BANNER=true upsdrvctl status
-	UPSNAME UPSDRV  PF_RUNNING      PF_PID  S_RESPONSIVE    S_PID   S_STATUS
-	dummy   dummy-ups       N/A     -3      NOT_RESPONSIVE  N/A
-	eco650  usbhid-ups      RUNNING 3559207 RESPONSIVE      3559207 "OL"
-	UPS2    dummy-ups       RUNNING 31455   RESPONSIVE      31455   "OL BOOST"
+	UPSNAME              UPSDRV  RUNNING PF_PID  S_RESPONSIVE    S_PID   S_STATUS
+	dummy             dummy-ups  N/A     -3      NOT_RESPONSIVE  N/A
+	eco650           usbhid-ups  RUNNING 3559207 RESPONSIVE      3559207 "OL"
+	UPS2              dummy-ups  RUNNING 31455   RESPONSIVE      31455   "OL BOOST"
 
-Values are TAB-separated. Any complex string values would be encased in
-double-quotes.
+Values are TAB-separated, but UPSNAME and UPSDRV may be padded by spaces
+on the right and on the left respectively. Any complex string values would
+be encased in double-quotes.
 
 Fields reported (`PF_*` = according to PID file, if any; `S_*` = via socket
 protocol):
 
         *UPSNAME*;;      driver section configuration name
         *UPSDRV*;;       driver program name per `ups.conf`
-        *PF_RUNNING*;;   `RUNNING` if `PF_PID` is valid (PID file existed,
-                         PID was parsed out of it and found running, program
-                         name corresponds to driver, etc.); `STOPPED` if PID
-                         was parsed but not running or has a wrong program
-                         name; "N/A" if failed to parse it or no PID file
+        *RUNNING*;;      `RUNNING` if `PF_PID` or `S_PID` is valid,
+                         `STOPPED` if at least one PID value was parsed but
+                         none was found running with a correct program name;
+                         `N/A` if no PID file/socket reply or failed to parse.
+                         First the PID file is consulted, but it may be absent
+                         either due to command-line parameters of daemons, or
+                         due to platform (WIN32). If no PID value was found and
+                         confirmed this way, we fall back to checking the PID
+                         reported via protocol (if available and different).
         *PF_PID*;;       PID of driver according to PID file (if any), or some
                          negative values upon errors (as defined in `common.c`)
+                         including an absent PID file, invalid contents, or
+                         unsupported platform for this mechanism (e.g. WIN32)
         *S_RESPONSIVE*;; `RESPONSIVE` if `PING`/`PONG` during socket protocol
                          session setup succeeded; `NOT_RESPONSIVE` otherwise
-        *S_PID*;;        PID of driver according to `GETPID` active query
-        *S_STATUS*;;     Value of `ups.status` variable
+        *S_PID*;;        PID of driver according to `GETPID` active query,
+                         or `N/A` if the query failed
+        *S_STATUS*;;     Quoted value of `ups.status` variable
 
 This mode does not discover drivers that are not in `ups.conf` (e.g. started
 manually for experiments with many `-x` CLI options).

docs/man/upsdrvsvcctl.txt

@@ -11,7 +11,7 @@ SYNOPSIS
 
 *upsdrvsvcctl* -h
 
-*upsdrvsvcctl* ['OPTIONS'] {start | stop } ['ups']
+*upsdrvsvcctl* ['OPTIONS'] {start | stop | status} ['ups']
 
 DESCRIPTION
 -----------
@@ -61,7 +61,13 @@ OPTIONS
 -------
 
 *-h*::
-Display the help text.
+Display the help text, including the built-in version of the script.
+
+*-V*::
+Display the version of NUT binaries (calling *upsdrvctl -V*), which
+normally should not differ much from the built-in version of the script
+shown in help. But with custom builds everything is possible, so it may
+be useful to know.
 
 *-t*::
 Enable testing mode. Testing mode makes upsdrvsvcctl display the actions
@@ -129,8 +135,11 @@ Stop the UPS driver(s).
 
 *status*::
 Query run-time status of all configured devices (or one specified device).
-Currently defers work to linkman:upsdrvctl[8], and does not report service
-unit information.
+Currently defers work to linkman:upsdrvctl[8], to list known device
+configurations and their driver daemon details (PID, responsiveness,
+`ups.status`) and to linkman:nut-driver-enumerator[8] to map device
+names to service unit instances to report their names and states in
+the service management framework.
 
 *upsdrvsvcctl* also supports further operations for troubleshooting the
 mapping of NUT driver section names to the service instance names (which

docs/sock-protocol.txt

@@ -162,6 +162,14 @@ This is sent in response to a PING from the server.  It is only used as
 a sanity check to make sure that the driver has not gotten stuck
 somewhere.
 
+OK
+~~
+
+	OK Goodbye
+
+This is sent in response to a LOGOUT from the server (or more likely from
+a sibling driver or `upsdrvctl` program).
+
 DATAOK
 ~~~~~~
 
@@ -312,6 +320,17 @@ numeric argument. Note that initial default is to receive everything, so
 this command may be useful for connections that disabled broadcasts at
 some point.
 
+LOGOUT
+~~~~~~
+
+Primarily used by communications between driver processes and/or `upsdrvctl`,
+this command allows clients to gracefully close connection to the NUT driver
+which acts as the server on the socket/pipe, avoiding noisy logs about sudden
+disconnection.
+
+	LOGOUT
+	OK Goodbye
+
 Design notes
 ------------
 

drivers/dstate.c

@@ -203,18 +203,22 @@ static TYPE_FD sock_open(const char *fn)
 static void sock_disconnect(conn_t *conn)
 {
 #ifndef WIN32
+	upsdebugx(3, "%s: disconnecting socket %d", __func__, (int)conn->fd);
 	close(conn->fd);
 #else
 	/* FIXME not sure if this is the right way to close a connection */
 	if( conn->read_overlapped.hEvent != INVALID_HANDLE_VALUE) {
 		CloseHandle(conn->read_overlapped.hEvent);
 		conn->read_overlapped.hEvent = INVALID_HANDLE_VALUE;
 	}
+	upsdebugx(3, "%s: disconnecting named pipe handle %p", __func__, conn->fd);
 	DisconnectNamedPipe(conn->fd);
 #endif
 
+	upsdebugx(5, "%s: finishing parsing context", __func__);
 	pconf_finish(&conn->ctx);
 
+	upsdebugx(5, "%s: relinking the chain of connections", __func__);
 	if (conn->prev) {
 		conn->prev->next = conn->next;
 	} else {
@@ -227,6 +231,7 @@ static void sock_disconnect(conn_t *conn)
 		/* conntail = conn->prev; */
 	}
 
+	upsdebugx(5, "%s: freeing the conn object", __func__);
 	free(conn);
 }
 
@@ -571,6 +576,7 @@ static void sock_connect(TYPE_FD sock)
 
 	conn->nobroadcast = 0;
 	conn->readzero = 0;
+	conn->closing = 0;
 	pconf_init(&conn->ctx, NULL);
 
 	if (connhead) {
@@ -701,6 +707,22 @@ static int sock_arg(conn_t *conn, size_t numarg, char **arg)
 		return 0;
 	}
 
+	if (!strcasecmp(arg[0], "LOGOUT")) {
+		send_to_one(conn, "OK Goodbye\n");
+#ifndef WIN32
+		upsdebugx(2, "%s: received LOGOUT on socket %d, will be disconnecting", __func__, (int)conn->fd);
+#else
+		upsdebugx(2, "%s: received LOGOUT on handle %p, will be disconnecting", __func__, conn->fd);
+#endif
+		/* Let the system flush the reply somehow (or the other
+		 * side to just see it) before we drop the pipe */
+		usleep(1000000);
+		/* err on the safe side, and actually close/free conn separately */
+		conn->closing = 1;
+		upsdebugx(4, "%s: LOGOUT processing finished", __func__);
+		return 2;
+	}
+
 	if (!strcasecmp(arg[0], "GETPID")) {
 		send_to_one(conn, "PID %" PRIiMAX "\n", (intmax_t)getpid());
 		return 1;
@@ -901,6 +923,7 @@ static int sock_arg(conn_t *conn, size_t numarg, char **arg)
 static void sock_read(conn_t *conn)
 {
 	ssize_t	ret, i;
+	int	ret_arg = -1;
 
 #ifndef WIN32
 	char	buf[SMALLBUF];
@@ -979,15 +1002,22 @@ static void sock_read(conn_t *conn)
 			continue;
 
 		case 1: /* try to use it, and complain about unknown commands */
-			if (!sock_arg(conn, conn->ctx.numargs, conn->ctx.arglist)) {
+			ret_arg = sock_arg(conn, conn->ctx.numargs, conn->ctx.arglist);
+			if (!ret_arg) {
 				size_t	arg;
 
 				upslogx(LOG_INFO, "Unknown command on socket: ");
 
 				for (arg = 0; arg < conn->ctx.numargs && arg < INT_MAX; arg++) {
 					upslogx(LOG_INFO, "arg %d: %s", (int)arg, conn->ctx.arglist[arg]);
 				}
+			} else if (ret_arg == 2) {
+				/* closed by LOGOUT processing, conn is free()'d */
+				if (i < ret)
+					upsdebugx(1, "%s: returning early, socket may be not valid anymore", __func__);
+				return;
 			}
+
 			continue;
 
 		default: /* nothing parsed */
@@ -1078,13 +1108,12 @@ int dstate_poll_fds(struct timeval timeout, TYPE_FD arg_extrafd)
 {
 	int	maxfd = 0; /* Unidiomatic use vs. "sockfd" below, which is "int" on non-WIN32 */
 	int	overrun = 0;
-	conn_t	*conn;
+	conn_t	*conn, *cnext;
 	struct timeval	now;
 
 #ifndef WIN32
 	int	ret;
 	fd_set	rfds;
-	conn_t	*cnext;
 
 	FD_ZERO(&rfds);
 	FD_SET(sockfd, &rfds);
@@ -1157,6 +1186,14 @@ int dstate_poll_fds(struct timeval timeout, TYPE_FD arg_extrafd)
 		}
 	}
 
+	for (conn = connhead; conn; conn = cnext) {
+		cnext = conn->next;
+
+		if (conn->closing) {
+			sock_disconnect(conn);
+		}
+	}
+
 	/* tell the caller if that fd woke up */
 	if (VALID_FD(arg_extrafd) && (FD_ISSET(arg_extrafd, &rfds))) {
 		return 1;
@@ -1238,6 +1275,14 @@ int dstate_poll_fds(struct timeval timeout, TYPE_FD arg_extrafd)
 		}
 	}
 
+	for (conn = connhead; conn; conn = cnext) {
+		cnext = conn->next;
+
+		if (conn->closing) {
+			sock_disconnect(conn);
+		}
+	}
+
 	/* tell the caller if that fd woke up */
 /*
 	if (VALID_FD(arg_extrafd) && (ret == arg_extrafd)) {

drivers/dstate.h

@@ -54,6 +54,7 @@ typedef struct conn_s {
 	struct conn_s	*next;
 	int	nobroadcast;	/* connections can request to ignore send_to_all() updates */
 	int	readzero;	/* how many times in a row we had zero bytes read; see DSTATE_CONN_READZERO_THROTTLE_USEC and DSTATE_CONN_READZERO_THROTTLE_MAX */
+	int	closing;	/* raised during LOGOUT processing, to close the socket when time is right */
 } conn_t;
 
 /* sleep after read()ing zero bytes */