[v3,2/4] tools/misc: xenwatchdogd enhancements

Commit Message

Leigh Brown April 11, 2024, 6:20 p.m. UTC

From: Leigh Brown <[e-mail]>

Add usage() function, the ability to run in the foreground, and
the ability to disarm the watchdog timer when exiting.

Add enhanced parameter parsing and validation, making use of
getopt_long().  Check the number of parameters are correct, the
timeout is at least two seconds (to allow a minimum sleep time of
one second), and that the sleep time is at least one and less
than the watchdog timeout.

With these changes, the daemon will no longer instantly reboot
the domain if you enter a zero timeout (or non-numeric parameter),
and prevent the daemon consuming 100% of a CPU due to zero sleep
time.

Signed-off-by: Leigh Brown <[e-mail]>
---
 tools/misc/xenwatchdogd.c | 94 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 10 deletions(-)

Comments

Anthony PERARD April 22, 2024, 5:10 p.m. UTC | #1

On Thu, Apr 11, 2024 at 07:20:21PM +0100, [e-mail] wrote:
> From: Leigh Brown <[e-mail]>
> 
> Add usage() function, the ability to run in the foreground, and
> the ability to disarm the watchdog timer when exiting.
> 
> Add enhanced parameter parsing and validation, making use of
> getopt_long().  Check the number of parameters are correct, the
> timeout is at least two seconds (to allow a minimum sleep time of
> one second), and that the sleep time is at least one and less
> than the watchdog timeout.
> 
> With these changes, the daemon will no longer instantly reboot
> the domain if you enter a zero timeout (or non-numeric parameter),
> and prevent the daemon consuming 100% of a CPU due to zero sleep
> time.
> 
> Signed-off-by: Leigh Brown <[e-mail]>

Reviewed-by: Anthony PERARD <[e-mail]>

Thanks,

Patch

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 9fa772e49f..a16d1efc13 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -10,6 +10,11 @@ 
 #include <signal.h>
 #include <stdio.h>
 #include <stdbool.h>
+#include <getopt.h>
+
+#define WDOG_MIN_TIMEOUT 2
+#define WDOG_MIN_SLEEP 1
+#define WDOG_EXIT_TIMEOUT 300
 
 static xc_interface *h;
 static volatile bool safeexit = false;
@@ -49,6 +54,26 @@  static void catch_usr1(int sig)
     done = true;
 }
 
+static void __attribute__((noreturn)) usage(int exit_code)
+{
+    FILE *out = exit_code ? stderr : stdout;
+
+    fprintf(out,
+	"Usage: xenwatchdog [OPTION]... <timeout> [<sleep>]\n"
+	"  -h, --help\t\tDisplay this help text and exit.\n"
+	"  -F, --foreground\tRun in foreground.\n"
+	"  -x, --safe-exit\tDisable watchdog on orderly exit.\n"
+	"\t\t\tNote: default is to set a %d second timeout on exit.\n\n"
+	"  timeout\t\tInteger seconds to arm the watchdog each time.\n"
+	"\t\t\tNote: minimum timeout is %d seconds.\n\n"
+	"  sleep\t\t\tInteger seconds to sleep between arming the watchdog.\n"
+	"\t\t\tNote: sleep must be at least %d and less than timeout.\n"
+	"\t\t\tIf not specified then set to half the timeout.\n",
+	WDOG_EXIT_TIMEOUT, WDOG_MIN_TIMEOUT, WDOG_MIN_SLEEP
+	);
+    exit(exit_code);
+}
+
 static int parse_secs(const char *arg, const char *what)
 {
     char *endptr;
@@ -66,22 +91,71 @@  int main(int argc, char **argv)
     int id;
     int t, s;
     int ret;
+    bool daemon = true;
+
+    for ( ;; )
+    {
+	int option_index = 0, c;
+	static const struct option long_options[] =
+	{
+	    { "help", no_argument, NULL, 'h' },
+	    { "foreground", no_argument, NULL, 'F' },
+	    { "safe-exit", no_argument, NULL, 'x' },
+	    { NULL, 0, NULL, 0 },
+	};
+
+	c = getopt_long(argc, argv, "hFxD", long_options, &option_index);
+	if (c == -1)
+	    break;
+
+	switch (c)
+	{
+	case 'h':
+	    usage(EXIT_SUCCESS);
+
+	case 'F':
+	    daemon = false;
+	    break;
+
+	case 'x':
+	    safeexit = true;
+	    break;
+
+	default:
+	    usage(EXIT_FAILURE);
+	}
+    }
 
-    if (argc < 2)
-	errx(EXIT_FAILURE, "usage: %s <timeout> <sleep>", argv[0]);
+    if (argc - optind < 1)
+	errx(EXIT_FAILURE, "timeout must be specified");
+
+    if (argc - optind > 2)
+	errx(EXIT_FAILURE, "too many arguments");
+
+    t = parse_secs(argv[optind], "timeout");
+    if (t < WDOG_MIN_TIMEOUT)
+	errx(EXIT_FAILURE, "Error: timeout must be at least %d seconds",
+			   WDOG_MIN_TIMEOUT);
+
+    ++optind;
+    if (optind < argc) {
+	s = parse_secs(argv[optind], "sleep");
+	if (s < WDOG_MIN_SLEEP)
+	    errx(EXIT_FAILURE, "Error: sleep must be no less than %d",
+			       WDOG_MIN_SLEEP);
+	if (s >= t)
+	    errx(EXIT_FAILURE, "Error: sleep must be less than timeout");
+    }
+    else
+	s = t / 2;
 
-    daemonize();
+    if (daemon)
+	daemonize();
 
     h = xc_interface_open(NULL, NULL, 0);
     if (h == NULL)
 	err(EXIT_FAILURE, "xc_interface_open");
 
-    t = parse_secs(argv[1], "timeout");
-
-    s = t / 2;
-    if (argc == 3)
-	s = parse_secs(argv[2], "sleep");
-
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
 	err(EXIT_FAILURE, "signal");
     if (signal(SIGINT, &catch_exit) == SIG_ERR)
@@ -105,6 +179,6 @@  int main(int argc, char **argv)
     }
 
     // Zero seconds timeout will disarm the watchdog timer
-    xc_watchdog(h, id, safeexit ? 0 : 300);
+    xc_watchdog(h, id, safeexit ? 0 : WDOG_EXIT_TIMEOUT);
     return 0;
 }