[Toybox] [PATCH] Introducing toys/pending/watchdog.c
enh
enh at google.com
Sat Aug 22 13:05:25 PDT 2020
On Sat, Aug 22, 2020 at 11:22 AM Chris Sarra via Toybox <
toybox at lists.landley.net> wrote:
> This patch introduces a simple watchdog implementation for toybox. We
> send the appropriate ioctls to set the relevant timeouts, and intercept
> signals to safely shut down if required.
> ---
> toys/pending/watchdog.c | 128 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
> create mode 100644 toys/pending/watchdog.c
>
> diff --git a/toys/pending/watchdog.c b/toys/pending/watchdog.c
> new file mode 100644
> index 00000000..0813fe69
> --- /dev/null
> +++ b/toys/pending/watchdog.c
> @@ -0,0 +1,128 @@
> +/* watchdog - start a watchdog timer with configurable kick frequencies
> +
> + Author: Chris Sarra, chrissarra at google.com
not sure why we're both working on a Saturday, but...
>
> + Date: 25 July 2019
> + Ref: kernel.org/doc/Documentation/watchdog/watchdog-api.txt
>
use the " * " indent from the other source files?
also mention that there's precedent in a busybox "watchdog", which this is
command-line compatible with?
> +USE_WATCHDOG(NEWTOY(watchdog, "Ft#T#", TOYFLAG_NEEDROOT|TOYFLAG_BIN))
>
huh. i didn't realize that USE_ worked _inside_ the comment! probably best
to move it below the comment as is the convention though.
> +config WATCHDOG
> + bool "watchdog"
> + default y
> + help
> + usage: watchdog [-F] [-t SW_TIMER_S] [-T HW_TIMER_S] DEV
> +
> + Start the watchdog timer at DEV with optional timeout parameters.
>
(blank line here.)
> + -F run in the foreground (do not daemonize)
> + -t software timer (in seconds)
> + -T hardware timer (in seconds)
>
say what the defaults are?
the busybox help implies that it's possible to use more precision than
seconds? afaik from the kernel source and kernel docs, though, that's a lie?
i think it would help to explain that -T is what you set the hardware
watchdog to, and -t is how often you kick it. i didn't get that from either
your --help or the busybox --help.
so something like:
-T N Set hardware watchdog to N seconds (default 60).
-t N Kick watchdog every N seconds (default 30).
? (note that it should be a tab between the "-T N" part and the "Set ..."
part.)
> +*/
> +#define FOR_watchdog
> +#include "toys.h"
> +#include "linux/watchdog.h"
> +
> +// Make sure no DEBUG variable is set; change this if you need debug
> prints!
> +#undef WATCHDOG_DEBUG
>
delete this or turn it into a -v flag, depending on whether it's still
useful now the toy has been written, or was just useful during development?
(rtcwake, for example, has this kind of thing in -v, but sleep doesn't.)
> +// Default timeout values in seconds.
> +#define WATCHDOG_SW_TIMER_S_DEFAULT (4)
> +#define WATCHDOG_HW_TIMER_S_DEFAULT (60)
>
is there a reason these are different from busybox's defaults, which seem
to be 30 and 60?
also, you can declare defaults in the USE_ line with =30 or =60, letting
you remove this and the corresponding code.
> +GLOBALS(
> + long hw_timer_s;
> + long sw_timer_s;
> + int fd;
> +)
> +
> +static int intercept_signals(void (*fn)(int)) {
> + int rc = 0;
> + struct sigaction sigact;
> + memset(&sigact, 0, sizeof(sigact));
> + sigact.sa_handler = fn;
> +
> + rc = sigaction(SIGTERM, &sigact, NULL);
> +#if defined(WATCHDOG_DEBUG)
> + if ( rc ) {
> + printf("failed to create new sigaction for SIGTERM: %d\n", rc);
> + }
> +#endif
> + return rc;
> +}
>
(this is already written. see call site below.)
> +void safe_shutdown(int __attribute__((unused))ignored) {
> + write(TT.fd, "V", 1);
> + close(TT.fd);
> + TT.fd = -1;
> + error_exit("safely exited watchdog.");
> +}
> +
> +void watchdog_main(void) {
> + int rc = 0;
> + long hw_timer_sec = 0;
> + char *watchdog_dev = NULL;
> +
> + if ( toys.optc > 0 ) {
> + watchdog_dev = toys.optargs[0];
> +#if defined(WATCHDOG_DEBUG)
> + printf("using dev @ '%s'\n", watchdog_dev);
> +#endif
> + } else {
> + error_exit("watchdog dev required");
> + }
>
you can replace all this with <1 at the start of the USE_TOY string. (see
sleep.c for an example.)
> + // Set default values for timeouts if no flags
> + if (!(toys.optflags & FLAG_t)) {
>
FLAG(t), but you can just put this in the USE_TOY as explained above.
> + TT.sw_timer_s = WATCHDOG_SW_TIMER_S_DEFAULT;
> +#if defined(WATCHDOG_DEBUG)
> + printf("using default sw_timer_s.\n");
> +#endif
> + }
> +
> + if (!(toys.optflags & FLAG_T)) {
> + TT.hw_timer_s = WATCHDOG_HW_TIMER_S_DEFAULT;
> +#if defined(WATCHDOG_DEBUG)
> + printf("using default hw_timer_s.\n");
> +#endif
> + }
> +
> +#if defined(WATCHDOG_DEBUG)
> + printf("hw timer: %ld seconds\n", TT.hw_timer_s);
> + printf("sw timer: %ld seconds\n", TT.sw_timer_s);
> +#endif
> +
> + if (!(toys.optflags & FLAG_F)) {
> +#if defined(WATCHDOG_DEBUG)
> + printf("daemonizing. so long, foreground!\n");
> +#endif
> + // Attempt to daemonize
> + rc = daemon(1, 1);
> + if ( rc ) {
> + perror_exit("failed to daemonize: %d", rc);
>
rc will always be -1 on failure, so all this can just be `if (!FLAG(F) &&
daemon(1, 1)) perror_exit("daemon failed");`.
> + }
> + }
> +
> + // Intercept terminating signals so we can call our shutdown routine
> first.
> + if ( intercept_signals(safe_shutdown) ) {
> + perror_exit("failed to intercept desired signals: %d", rc);
> + }
> +#if defined(WATCHDOG_DEBUG)
> + printf("Successfully intercepted signals.\n");
> +#endif
>
sigatexit()
> + TT.fd = open(watchdog_dev, O_WRONLY);
> + if ( TT.fd == -1 ) {
> + perror_exit("failed to open '%s'", watchdog_dev);
> + }
>
xopen()
> +#if defined(WDIOC_SETTIMEOUT)
>
as far as i can tell, this has existed since 2.6 kernels, so you don't need
the #if?
> + // SETTIMEOUT takes time value in seconds: s = ms / (1000 ms/s)
> + hw_timer_sec = TT.hw_timer_s;
> + xioctl(TT.fd, WDIOC_SETTIMEOUT, (void *)&hw_timer_sec);
>
(no need for this local if you let USE_TOY do the default for you. even
when you can't, you can just assign to TT.whatever.)
> +#endif // WDIOC_SETTIMEOUT
> +
> + // Now that we've got the watchdog device open, kick it periodically.
> + while (1) {
> + write(TT.fd, "\0", 1);
>
xwrite?
> + usleep(TT.sw_timer_s * 1000 * 1000);
>
isn't this just sleep(TT.sw_timer_s)?
> + }
> +}
> --
> 2.28.0.297.g1956fa8f8d-goog
>
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20200822/6287f017/attachment.htm>
More information about the Toybox
mailing list