[Toybox] [PATCH] Introducing toys/pending/watchdog.c

[Rob Landley]

On 8/22/20 1:22 PM, Chris Sarra via Toybox 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

This is small enough I can clean up and promote it quickly. Below are the things
I fixed, let me know if the result still works for you?

$ git diff toys/*/watchdog.c | diffstat
 watchdog.c |  120 ++++++++++---------------------------------------------------
 1 file changed, 20 insertions(+), 100 deletions(-)

> 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
> + Date: 25 July 2019
> + Ref: kernel.org/doc/Documentation/watchdog/watchdog-api.txt

Doing a style change here to match other commands.

> +USE_WATCHDOG(NEWTOY(watchdog, "Ft#T#", TOYFLAG_NEEDROOT|TOYFLAG_BIN))

Are there minimums on t and T? (I'm assuming 1?)

> +config WATCHDOG
> +  bool "watchdog"
> +  default y

commands in pending default n

> +  help
> +    usage: watchdog [-F] [-t SW_TIMER_S] [-T HW_TIMER_S] DEV
> +
> +    Start the watchdog timer at DEV with optional timeout parameters.
> +    -F run in the foreground (do not daemonize)
> +    -t software timer (in seconds)
> +    -T hardware timer (in seconds)

We usually have a tab between the option its description in option lines, and a
blank line between the command description and the options.

> +*/
> +#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

I tend not to keep debug code in finished commands. (Both because it's easy to
stick in new printfs when you need to track down a bug, and because debug code
only ever means stuff to the original developer.)

> +// Default timeout values in seconds.
> +#define WATCHDOG_SW_TIMER_S_DEFAULT (4)
> +#define WATCHDOG_HW_TIMER_S_DEFAULT (60)

Each is only used once so would be inlined. (I'm aware of the desire to move
tuning variables out to the start, but A) are they ever going to change? and B)
it's easy enough to find them in-situ.)

But "would be" here means I can just move this to the defaults set in the optstr
ala "t#=4T#=60" and not test for them at all in main.

> +GLOBALS(
> +  long hw_timer_s;
> +  long sw_timer_s;
> +  int fd;
> +)

Code style: we usually use the name of the argument as the name of the variable
set by the argument, so that would be long T, t;

Code style: we generally have a blank line between variables set by argument
parsing and variables NOT set by it.

> +
> +static int intercept_signals(void (*fn)(int)) {

Code style: curly bracket on its own line at the start of a function. (Blame
Kernighan.)

(The function name is plural but you're only interecepting one signal?)

> +  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;
> +}

We have xsignal() in lib/xwrap.c that basically does this, except it exits
instead of just warning on failure. (Note: the man page says the only 2 failures
are EFAULT for feeding it a pointer to bad memory, and EINVAL for invalid signal
number. So unless selinux is doing something weird here xsignal(SIGTERM,
valid_function_pointer) should never fail anyway.

> +void safe_shutdown(int __attribute__((unused))ignored) {

I'm pretty sure the optimizer can figure out that the 4 line function doesn't
use its argument.

> +  write(TT.fd, "V", 1);
> +  close(TT.fd);
> +  TT.fd = -1;
> +  error_exit("safely exited watchdog.");
> +}

Don't need to set TT.fd = -1 here. If you closed it, you can't write two V's to
the device even if signals double up somehow (how?), and if the signal reentered
between the write and the close setting -1 _after_ the close you apparently
didn't reach wouldn't stop it anyway?

I _suspect_ you don't need the close() either, since exiting will do it for you,
but I dunno the watchdog corner cases and won't touch that one in case there's
persnickety driver magic here?

> +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");
> +  }

So... you can just use *toys.optargs without having to copy it to the a local
variable because when it isn't set from this one place we exited? And the optstr
can require exactly one argument with <1>1 at the start of the string (at least
one, at most one. I've thought about implementing =1 here but it doesn't same
much and later =1 assigns rather than limits, so might be confusing?)

> +  // Set default values for timeouts if no flags
> +  if (!(toys.optflags & FLAG_t)) {
> +    TT.sw_timer_s = WATCHDOG_SW_TIMER_S_DEFAULT;
> +#if defined(WATCHDOG_DEBUG)
> +    printf("using default sw_timer_s.\n");
> +#endif
> +  }

Moved to t#=4<1 in optstr

> +  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
> +  }

Moved to T#=60<1 in optstr.

Hmmm, can we -T0 legitimately? -t 0 would spin eating cpu and you didn't
implement -t .5 so I didn't either, although this is sort of what % arguments
are for except if -T doesn't implement it having -t do so is confusing, so for
consistency what's there makes sense. And -T 0 would logically disable the
hardware watchdog, meaning this wouldn't DO anything. (Or should -T 0 disable
the ioctl call after we opened it? Some watchdog hardware might have a default
or something just from being opened? Dunno. For now minimums of 1 for both seem
to make sense.)

> +#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

Yanked debug code.

> +  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);
> +    }
> +  }

Becomes just;

  if (!FLAG(F) && daemon(1, 1)) perror_exit("failed to daemonize");

The man page says rc is always 0 or -1, errno is the granular info, and that's
the p in perror (just error_exit() doesn't include the ":%s", strerror(errno) part).

> +  // 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

Removing the function it calls, this becomes:

  xsignal(SIGTERM, safe_shutdown);

> +  TT.fd = open(watchdog_dev, O_WRONLY);
> +  if ( TT.fd == -1 ) {
> +    perror_exit("failed to open '%s'", watchdog_dev);
> +  }

We have xopen() which perror_exits with message about filename it couldn't open.

> +#if defined(WDIOC_SETTIMEOUT)

Will this ever NOT be defined in a build environment we care about? (And if it
was, would you enable this command in the config?)

Bionic's libc/kernel/uapi/linux/watchdog.h #defines WDIOC_SETTIMEOUT _IOWR(...)
unconditionally, with no #ifdefs except the file's double inclusion guard.

> +  // 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);

hw_timer_sec is long, TT.T is long, might as well just feed &TT.T here? (This is
the only user so we don't even care if it modifies it.)

The argument type for xioctl is already void *, you don't have to typecast it in
C (just in C++).

> +#endif // WDIOC_SETTIMEOUT
> +
> +  // Now that we've got the watchdog device open, kick it periodically.
> +  while (1) {

trivial style thing: we use for (;;)

> +    write(TT.fd, "\0", 1);
> +    usleep(TT.sw_timer_s * 1000 * 1000);

Both sleep() and usleep() are wrappers around nanosleep (in both musl and bionic
source), so might as well just call sleep() here.

> +  }
> +}
> 

I was bad and stuck the TT.fd = xopen() into the xioctl() first argument (still
fit in 80 lines). I wouldn't have done this if the signal handler was set up
between the open() and the ioctl(), but it already wasn't.

This means technically there's a one line race where ctrl-c would write "V" to
stdin, but it was already there, and not a big deal. The alternative is the one
line race where ctrl-C could happen when the watchdog device had been opened
(but not ioctl()ed) and wouldn't have V written to it on the way out, which
MIGHT reboot the box? (Dunno.) The spurious ctrl-V written to stdin is harmless,
doesn't seem worth an if() in the signal handler...

Did I break it? I don't have a test thing here, although I could probably carve
one out of mkroot/qemu if I tried?

Rob