[PATCH] tools/misc: fix xenwatchdogd invocation

[PATCH] tools/misc: fix xenwatchdogd invocation

[zithro / Cyril Rébert]

When xenwatchdogd is invoked with -h/--help (or any non-number), the
argument value is converted to 0, which immediately shutdowns dom0.

So make sure only numbers are passed to the program, otherwise fail.
For the help, use getopt_long as suggested.
While there, reformat the code a bit (s/tabs/spaces/, indentation, etc).

Bug fix only, no functional change intended.

Reported-by: Leigh Brown <leigh@solinno.co.uk>
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Cyril Rébert / zithro <slack@rabbit.lu>
---
- Not sure about the preprocessor stanzas, copied them from xentop.
- A small explanation about what the program does could be helpful,
  like some kind of synopsis ? Purpose, gotchas, etc. I can do the
  writing, but please be specific !
- Built on 4.17 and unstable, tested on 4.17.
---
 tools/misc/xenwatchdogd.c | 77 +++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 20 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 254117b554..6ef5eaf45c 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -8,26 +8,34 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <signal.h>
+#include <string.h>
 #include <stdio.h>
 
+#define _GNU_SOURCE
+#include <getopt.h>
+#if !defined(__GNUC__) && !defined(__GNUG__)
+#define __attribute__(arg) /* empty */
+#endif
+
 xc_interface *h;
 int id = 0;
 
 void daemonize(void)
 {
     switch (fork()) {
-    case -1:
-	err(1, "fork");
-    case 0:
-	break;
-    default:
-	exit(0);
+        case -1:
+            err(1, "fork");
+        case 0:
+            break;
+        default:
+            exit(0);
     }
+
     umask(0);
     if (setsid() < 0)
-	err(1, "setsid");
+        err(1, "setsid");
     if (chdir("/") < 0)
-	err(1, "chdir /");
+        err(1, "chdir /");
     if (freopen("/dev/null", "r", stdin) == NULL)
         err(1, "reopen stdin");
     if(freopen("/dev/null", "w", stdout) == NULL)
@@ -52,39 +60,68 @@ void catch_usr1(int sig)
 
 int main(int argc, char **argv)
 {
+
     int t, s;
     int ret;
+    
+    char *usage = "usage: %s <timeout> <sleep>";
+    int opt, optind = 0;
+
+    struct option lopts[] = {
+        { "help",          no_argument,       NULL, 'h' },
+    };
+    const char *sopts = "h";
+
+    while ((opt = getopt_long(argc, argv, sopts, lopts, &optind)) != -1) {
+        switch (opt) {
+        case '?':
+        case 'h':
+            errx(1, usage, argv[0]);
+            break;
+        default:
+            errx(1, usage, argv[0]);
+        }
+    }
 
     if (argc < 2)
-	errx(1, "usage: %s <timeout> <sleep>", argv[0]);
+        errx(1, usage, argv[0]);
 
     daemonize();
 
     h = xc_interface_open(NULL, NULL, 0);
     if (h == NULL)
-	err(1, "xc_interface_open");
+        err(1, "xc_interface_open");
+
+    t = strtoul(argv[1], &argv[1], 0);
+    
+    // argv1 NaN
+    if ( *argv[1] != '\0' )
+        errx(1, "Error: timeout must be a number, got '%s'", argv[1]);
 
-    t = strtoul(argv[1], NULL, 0);
     if (t == ULONG_MAX)
-	err(1, "strtoul");
+        err(1, "strtoul");
 
     s = t / 2;
     if (argc == 3) {
-	s = strtoul(argv[2], NULL, 0);
-	if (s == ULONG_MAX)
-	    err(1, "strtoul");
+        s = strtoul(argv[2], &argv[2], 0);
+        // argv2 NaN
+        if ( *argv[2] != '\0' ){
+            errx(1, "Error: sleep must be a number, got '%s'", argv[2]);
+        }
+        if (s == ULONG_MAX)
+            err(1, "strtoul");
     }
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGINT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGQUIT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGTERM, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
     if (signal(SIGUSR1, &catch_usr1) == SIG_ERR)
-	err(1, "signal");
+        err(1, "signal");
 
     id = xc_watchdog(h, 0, t);
     if (id <= 0)
-- 
2.39.2

Re: [PATCH] tools/misc: fix xenwatchdogd invocation

[Jan Beulich]

On 26.03.2024 06:15, zithro / Cyril Rébert wrote:
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -8,26 +8,34 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <signal.h>
> +#include <string.h>
>  #include <stdio.h>
>  
> +#define _GNU_SOURCE

This wants defining first thing in a file (or even on the compiler command
line), to (properly) affect all headers.

> +#include <getopt.h>
> +#if !defined(__GNUC__) && !defined(__GNUG__)

Why __GNUG__ in a C file? And anyway, why is this construct needed? You
don't ...

> +#define __attribute__(arg) /* empty */

... use any attributes down from here.

You mention xentop as where you've taken them from. I view those as
questionable. If in fact you had used any other utility's source for
reference, you wouldn't have encountered such.

> +#endif
> +
>  xc_interface *h;
>  int id = 0;
>  
>  void daemonize(void)
>  {
>      switch (fork()) {
> -    case -1:
> -	err(1, "fork");
> -    case 0:
> -	break;
> -    default:
> -	exit(0);
> +        case -1:
> +            err(1, "fork");
> +        case 0:
> +            break;
> +        default:
> +            exit(0);
>      }

The case labels were properly indented, weren't they? And all other re-
indentation you do wants splitting from the functional change.

> @@ -52,39 +60,68 @@ void catch_usr1(int sig)
>  
>  int main(int argc, char **argv)
>  {
> +
>      int t, s;

What would this new blank line be good for?

>      int ret;
> +    

I'd even question this one.

> +    char *usage = "usage: %s <timeout> <sleep>";

static const char[]

> +    int opt, optind = 0;
> +
> +    struct option lopts[] = {

static const

> +        { "help",          no_argument,       NULL, 'h' },
> +    };
> +    const char *sopts = "h";
> +
> +    while ((opt = getopt_long(argc, argv, sopts, lopts, &optind)) != -1) {
> +        switch (opt) {
> +        case '?':
> +        case 'h':
> +            errx(1, usage, argv[0]);

This isn't an error and hence wants to produce an exit code of 0.

> +            break;
> +        default:
> +            errx(1, usage, argv[0]);
> +        }

Please be consistent with break statements: Either you omit them
uniformly when a noreturn function is call, or you put them there
in all cases.

> +    }
>  
>      if (argc < 2)
> -	errx(1, "usage: %s <timeout> <sleep>", argv[0]);
> +        errx(1, usage, argv[0]);
>  
>      daemonize();
>  
>      h = xc_interface_open(NULL, NULL, 0);
>      if (h == NULL)
> -	err(1, "xc_interface_open");
> +        err(1, "xc_interface_open");
> +
> +    t = strtoul(argv[1], &argv[1], 0);
> +    
> +    // argv1 NaN

NaN is a term normally used for floating point values only.

> +    if ( *argv[1] != '\0' )
> +        errx(1, "Error: timeout must be a number, got '%s'", argv[1]);

This still doesn't guarantee a numeric: An empty string as argument
would still yield a value of 0 if I'm not mistaken. As would a
string consisting of just blanks.

> -    t = strtoul(argv[1], NULL, 0);
>      if (t == ULONG_MAX)
> -	err(1, "strtoul");
> +        err(1, "strtoul");
>  
>      s = t / 2;
>      if (argc == 3) {
> -	s = strtoul(argv[2], NULL, 0);
> -	if (s == ULONG_MAX)
> -	    err(1, "strtoul");
> +        s = strtoul(argv[2], &argv[2], 0);
> +        // argv2 NaN
> +        if ( *argv[2] != '\0' ){
> +            errx(1, "Error: sleep must be a number, got '%s'", argv[2]);
> +        }

Like above you don't really need braces here. If, however, you add
them, the opening one wants to be separated by a blank from the
closing parenthesis. And following other style in this file, there
do not want to be blanks immediately inside the parentheses.

Jan

[PATCH 0/6] xenwatchdogd enhancements

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Following up on Cyril's email. I had been independently looking at this,
mainly because xenwatchdogd is simple enough for me to understand. The
primary intention of this patch series is to replace the pathologically
bad behaviour of rebooting the domain if you run "xenwatchdogd -h". To
that end, I have implemented comprehensive argument validation. This
validation ensures you can't pass arguments that instantly reboot the
domain or cause it to spin loop running sleep(0) repeatedly. I added a
couple of enhancements whilst working on the changes as they were easy
enough.

Full list of changes:
- Use getopt_long() to add -h/--help with associated usage help.
- Add -F/--foreground parameter to run without daemonizing.
- Add -x/--save-exit parameter to disarm the watchdog when exiting.
- Validate timeout is numeric and is at least two seconds.
- Validate sleep is numeric and is at least one and less than timeout.
- Check for too many arguments.
- Use symbol constants instead of magic numbers where possible.
- Add a manual page for xenwatchdogd().

I am not an expert in git or sending patches so forgive me if things
don't look quite right.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>

Leigh Brown (6):
  tools/misc: xenwatchdogd: use EXIT_* constants
  tools/misc: rework xenwatchdogd signal handling
  tools/misc: xenwatchdogd: make functions static
  tools/misc: xenwatchdogd: add parse_secs()
  tools/misc: xenwatchdogd enhancements
  docs/man: Add xenwatchdog manual page

 docs/man/xenwatchdogd.8.pod |  54 +++++++++++
 tools/misc/xenwatchdogd.c   | 180 ++++++++++++++++++++++++++++--------
 2 files changed, 195 insertions(+), 39 deletions(-)
 create mode 100644 docs/man/xenwatchdogd.8.pod

-- 
2.39.2

[PATCH 1/6] tools/misc: xenwatchdogd: use EXIT_* constants

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Use EXIT_SUCCESS/EXIT_FAILURE constants instead of magic numbers.
---
 tools/misc/xenwatchdogd.c | 40 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 254117b554..2f7c822d61 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -17,37 +17,37 @@ void daemonize(void)
 {
     switch (fork()) {
     case -1:
-	err(1, "fork");
+	err(EXIT_FAILURE, "fork");
     case 0:
 	break;
     default:
-	exit(0);
+	exit(EXIT_SUCCESS);
     }
     umask(0);
     if (setsid() < 0)
-	err(1, "setsid");
+	err(EXIT_FAILURE, "setsid");
     if (chdir("/") < 0)
-	err(1, "chdir /");
+	err(EXIT_FAILURE, "chdir /");
     if (freopen("/dev/null", "r", stdin) == NULL)
-        err(1, "reopen stdin");
+        err(EXIT_FAILURE, "reopen stdin");
     if(freopen("/dev/null", "w", stdout) == NULL)
-        err(1, "reopen stdout");
+        err(EXIT_FAILURE, "reopen stdout");
     if(freopen("/dev/null", "w", stderr) == NULL)
-        err(1, "reopen stderr");
+        err(EXIT_FAILURE, "reopen stderr");
 }
 
 void catch_exit(int sig)
 {
     if (id)
         xc_watchdog(h, id, 300);
-    exit(0);
+    exit(EXIT_SUCCESS);
 }
 
 void catch_usr1(int sig)
 {
     if (id)
         xc_watchdog(h, id, 0);
-    exit(0);
+    exit(EXIT_SUCCESS);
 }
 
 int main(int argc, char **argv)
@@ -56,44 +56,44 @@ int main(int argc, char **argv)
     int ret;
 
     if (argc < 2)
-	errx(1, "usage: %s <timeout> <sleep>", argv[0]);
+	errx(EXIT_FAILURE, "usage: %s <timeout> <sleep>", argv[0]);
 
     daemonize();
 
     h = xc_interface_open(NULL, NULL, 0);
     if (h == NULL)
-	err(1, "xc_interface_open");
+	err(EXIT_FAILURE, "xc_interface_open");
 
     t = strtoul(argv[1], NULL, 0);
     if (t == ULONG_MAX)
-	err(1, "strtoul");
+	err(EXIT_FAILURE, "strtoul");
 
     s = t / 2;
     if (argc == 3) {
 	s = strtoul(argv[2], NULL, 0);
 	if (s == ULONG_MAX)
-	    err(1, "strtoul");
+	    err(EXIT_FAILURE, "strtoul");
     }
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+	err(EXIT_FAILURE, "signal");
     if (signal(SIGINT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+	err(EXIT_FAILURE, "signal");
     if (signal(SIGQUIT, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+	err(EXIT_FAILURE, "signal");
     if (signal(SIGTERM, &catch_exit) == SIG_ERR)
-	err(1, "signal");
+	err(EXIT_FAILURE, "signal");
     if (signal(SIGUSR1, &catch_usr1) == SIG_ERR)
-	err(1, "signal");
+	err(EXIT_FAILURE, "signal");
 
     id = xc_watchdog(h, 0, t);
     if (id <= 0)
-        err(1, "xc_watchdog setup");
+        err(EXIT_FAILURE, "xc_watchdog setup");
 
     for (;;) {
         sleep(s);
         ret = xc_watchdog(h, id, t);
         if (ret != 0)
-            err(1, "xc_watchdog");
+            err(EXIT_FAILURE, "xc_watchdog");
     }
 }
-- 
2.39.2

[PATCH 2/6] tools/misc: rework xenwatchdogd signal handling

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Rework xenwatchdogd signal handling to do the minimum in the signal
handler. This is a very minor enhancement.
---
 tools/misc/xenwatchdogd.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 2f7c822d61..d4da0ad0b6 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -9,9 +9,11 @@
 #include <unistd.h>
 #include <signal.h>
 #include <stdio.h>
+#include <stdbool.h>
 
 xc_interface *h;
-int id = 0;
+bool safeexit = false;
+bool done = false;
 
 void daemonize(void)
 {
@@ -38,20 +40,18 @@ void daemonize(void)
 
 void catch_exit(int sig)
 {
-    if (id)
-        xc_watchdog(h, id, 300);
-    exit(EXIT_SUCCESS);
+    done = true;
 }
 
 void catch_usr1(int sig)
 {
-    if (id)
-        xc_watchdog(h, id, 0);
-    exit(EXIT_SUCCESS);
+    safeexit = true;
+    done = true;
 }
 
 int main(int argc, char **argv)
 {
+    int id;
     int t, s;
     int ret;
 
@@ -90,10 +90,14 @@ int main(int argc, char **argv)
     if (id <= 0)
         err(EXIT_FAILURE, "xc_watchdog setup");
 
-    for (;;) {
+    while (!done) {
         sleep(s);
         ret = xc_watchdog(h, id, t);
         if (ret != 0)
             err(EXIT_FAILURE, "xc_watchdog");
     }
+
+    // Zero seconds timeout will disarm the watchdog timer
+    xc_watchdog(h, id, safeexit ? 0 : 300);
+    return 0;
 }
-- 
2.39.2

[PATCH 3/6] tools/misc: xenwatchdogd: make functions static

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Make all functions except main() static in xenwatchdogd.c.
---
 tools/misc/xenwatchdogd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index d4da0ad0b6..224753e824 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -15,7 +15,7 @@ xc_interface *h;
 bool safeexit = false;
 bool done = false;
 
-void daemonize(void)
+static void daemonize(void)
 {
     switch (fork()) {
     case -1:
@@ -38,12 +38,12 @@ void daemonize(void)
         err(EXIT_FAILURE, "reopen stderr");
 }
 
-void catch_exit(int sig)
+static void catch_exit(int sig)
 {
     done = true;
 }
 
-void catch_usr1(int sig)
+static void catch_usr1(int sig)
 {
     safeexit = true;
     done = true;
-- 
2.39.2

[PATCH 4/6] tools/misc: xenwatchdogd: add parse_secs()

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Create a new parse_secs() function to parse the timeout and sleep
parameters. This ensures that non-numeric parameters are not
accidentally treated as numbers.
---
 tools/misc/xenwatchdogd.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 224753e824..26bfdef3db 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -49,6 +49,18 @@ static void catch_usr1(int sig)
     done = true;
 }
 
+static int parse_secs(const char *arg, const char *what)
+{
+    char *endptr;
+    unsigned long val;
+
+    val = strtoul(arg, &endptr, 0);
+    if (val > INT_MAX || *endptr != 0)
+	errx(EXIT_FAILURE, "invalid %s: '%s'", what, arg);
+
+    return val;
+}
+
 int main(int argc, char **argv)
 {
     int id;
@@ -64,16 +76,11 @@ int main(int argc, char **argv)
     if (h == NULL)
 	err(EXIT_FAILURE, "xc_interface_open");
 
-    t = strtoul(argv[1], NULL, 0);
-    if (t == ULONG_MAX)
-	err(EXIT_FAILURE, "strtoul");
+    t = parse_secs(argv[optind], "timeout");
 
     s = t / 2;
-    if (argc == 3) {
-	s = strtoul(argv[2], NULL, 0);
-	if (s == ULONG_MAX)
-	    err(EXIT_FAILURE, "strtoul");
-    }
+    if (argc == 3)
+	s = parse_secs(argv[optind], "sleep");
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
 	err(EXIT_FAILURE, "signal");
-- 
2.39.2

[PATCH 5/6] tools/misc: xenwatchdogd enhancements

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Add enhanced parameter parsing and validation, making use of
getopt_long(). Adds usage() function, ability to run in the foreground,
and the ability to disarm the watchdog timer when exiting.  Now checks
the number of parameters are correct, that 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. After
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. Add a copyright message. This is
based on the previous commits which were from Citrix email addresses.
---
 tools/misc/xenwatchdogd.c | 111 ++++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 10 deletions(-)

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 26bfdef3db..77638a4309 100644
--- a/tools/misc/xenwatchdogd.c
+++ b/tools/misc/xenwatchdogd.c
@@ -1,3 +1,20 @@
+/*
+ * xenwatchdogd.c
+ *
+ * Watchdog based on Xen hypercall watchdog interface
+ *
+ * Copyright 2010-2024 Citrix Ltd and other contributors
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
 
 #include <err.h>
 #include <limits.h>
@@ -10,6 +27,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
 
 xc_interface *h;
 bool safeexit = false;
@@ -49,6 +71,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,21 +108,70 @@ 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");
 
-    daemonize();
-
-    h = xc_interface_open(NULL, NULL, 0);
-    if (h == NULL)
-	err(EXIT_FAILURE, "xc_interface_open");
+    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);
 
-    s = t / 2;
-    if (argc == 3)
+    ++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;
+
+    if (daemon)
+	daemonize();
+
+    h = xc_interface_open(NULL, NULL, 0);
+    if (h == NULL)
+	err(EXIT_FAILURE, "xc_interface_open");
 
     if (signal(SIGHUP, &catch_exit) == SIG_ERR)
 	err(EXIT_FAILURE, "signal");
@@ -105,6 +196,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;
 }
-- 
2.39.2

[PATCH 6/6] docs/man: Add xenwatchdog manual page

[leigh]

From: Leigh Brown <leigh@solinno.co.uk>

Add a manual page for xenwatchdogd.
---
 docs/man/xenwatchdogd.8.pod | 54 +++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 docs/man/xenwatchdogd.8.pod

diff --git a/docs/man/xenwatchdogd.8.pod b/docs/man/xenwatchdogd.8.pod
new file mode 100644
index 0000000000..2f6454f183
--- /dev/null
+++ b/docs/man/xenwatchdogd.8.pod
@@ -0,0 +1,54 @@
+=head1 NAME
+
+xenwatchdogd - Xen hypercall based watchdog daemon
+
+=head1 SYNOPSIS
+
+B<xenwatchdogd> [ I<OPTIONS> ] <I<TIMEOUT>> [ <I<SLEEP>> ]
+
+=head1 DESCRIPTION
+
+B<xenwatchdogd> arms the Xen watchdog timer to I<TIMEOUT> every I<SLEEP>
+seconds. If the xenwatchdogd process dies or is delayed for more than
+I<TIMEOUT> seconds, then Xen will reboot the domain. If the domain being
+rebooted is domain 0, the whole system will reboot.
+
+=head1 OPTIONS
+
+=over 4
+
+=item B<-h>, B<--help>
+
+Display a help message.
+
+=item B<-F>, B<--foreground>
+
+Run in the foreground. The default behaviour is to daemonize.
+
+=item B<-x>, B<--safe-exit>
+
+Disable watchdog on orderly exit. The default behaviour is to arm the
+watchdog to 300 seconds to allow time for the domain to shutdown.  See 
+also the B<SIGNALS> section.
+
+=item B<timeout>
+
+The number of seconds to arm the Xen watchdog timer. This must be set to
+a minimum of two.
+
+=item B<sleep>
+
+The number of seconds to sleep in between calls to arm the Xen watchdog
+timer. This must be at least one second, and less than the I<timeout>
+value. If not specified, it defaults to half the I<timeout> value.
+
+=back
+
+=head1 SIGNALS
+
+B<SIGUSR1> Will cause the program to disarm the watchdog timer and exit,
+regardless of whether the safe exit option was passed.
+
+=head1 AUTHOR
+
+Citrix Ltd and other contributors.
-- 
2.39.2

Re: [PATCH 2/6] tools/misc: rework xenwatchdogd signal handling

[Jan Beulich]

On 27.03.2024 19:13, leigh@solinno.co.uk wrote:
> From: Leigh Brown <leigh@solinno.co.uk>
> 
> Rework xenwatchdogd signal handling to do the minimum in the signal
> handler. This is a very minor enhancement.
> ---
>  tools/misc/xenwatchdogd.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

Throughout the series Signed-off-by: are missing from both you and Leigh.
The latter you may of course add only in case of this either having been
provided earlier (and dropped for an unknown reason), or with respective
agreement.

> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -9,9 +9,11 @@
>  #include <unistd.h>
>  #include <signal.h>
>  #include <stdio.h>
> +#include <stdbool.h>
>  
>  xc_interface *h;
> -int id = 0;
> +bool safeexit = false;
> +bool done = false;

Seeing the subsequent patch adding static, please don't introduce new
non-static items.

Jan

Re: [PATCH 3/6] tools/misc: xenwatchdogd: make functions static

[Jan Beulich]

On 27.03.2024 19:13, leigh@solinno.co.uk wrote:
> From: Leigh Brown <leigh@solinno.co.uk>
> 
> Make all functions except main() static in xenwatchdogd.c.

And once at it data then too, please.

Jan

Re: [PATCH 2/6] tools/misc: rework xenwatchdogd signal handling

[Jan Beulich]

On 28.03.2024 10:31, Jan Beulich wrote:
> On 27.03.2024 19:13, leigh@solinno.co.uk wrote:
>> From: Leigh Brown <leigh@solinno.co.uk>
>>
>> Rework xenwatchdogd signal handling to do the minimum in the signal
>> handler. This is a very minor enhancement.
>> ---
>>  tools/misc/xenwatchdogd.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> Throughout the series Signed-off-by: are missing from both you and Leigh.
> The latter you may of course add only in case of this either having been
> provided earlier (and dropped for an unknown reason), or with respective
> agreement.

Correction: I was misguided by the mention of someone else's name in the
cover letter. It's really you who is the author aiui. And it's just that
the S-o-b doesn't belong in the cover letter, but on every individual
patch.

Jan