diff mbox series

[v2,5/6] tools/misc: xenwatchdogd enhancements

Message ID 20240329111056.6118-6-leigh@solinno.co.uk (mailing list archive)
State Superseded
Headers show
Series xenwatchdogd bugfixes and enhancements | expand

Commit Message

Leigh Brown March 29, 2024, 11:10 a.m. UTC
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.

Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
 tools/misc/xenwatchdogd.c | 111 ++++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 10 deletions(-)

Comments

Anthony PERARD April 9, 2024, 4:46 p.m. UTC | #1
On Fri, Mar 29, 2024 at 11:10:55AM +0000, leigh@solinno.co.uk wrote:
> 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.

This to me is really hard to read, it just looks like a blob of text,
where it supposed to be a list with several modification listed. The
part about the copyright should be in its own paragraph for example.


> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> ---
>  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 19ec4c5359..b78320f86d 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 is probably more like:
Copyright (C) 2010 Citrix Ltd.
Copyright (C) 2024 *** your copyright here ***

Because it's looks like the only contribution from us was in 2010, and I
suppose it's fine to have more than one copyright line.

> + * 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.

This might be the wrong license, the default license we use is GPL 2.0
only, not LGPL. See :/COPYING .

These days, we prefer SPDX tags instead of the full licence text.

So overall, the header of the file should look something like:

/* SPDX-License-Identifier: GPL-2.0-only */
/*
 * xenwatchdogd.c
 * 
 * Watchdog based on Xen hypercall watchdog interface
 * 
 * Copyright (C) 2010 Citrix Ltd.
 * Copyright (C) 2024 *** your copyright here ***
 */

I don't know if adding the file name in that header is very useful, but
I don't mind either way.

Also, could you do this in a separate patch?

> + */
>  
>  #include <err.h>
>  #include <limits.h>

Nice change overall, it's just the license part that need fixing.

Thanks,
diff mbox series

Patch

diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
index 19ec4c5359..b78320f86d 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
 
 static xc_interface *h;
 static 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;
 }