[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