Use two timers and sigwait rather than looking for EINTR.

The latter had a race condition whereby if the signal from the
fallback timer was delivered before the call to clock_nanosleep(), it
would be lost.  This was a fundamental problem with using asynchronous
signals, and switching to sigwait(), which can wait for blocked signals,
should solve it.

diff --git a/clunk.c b/clunk.c
index 1879e708d363a001636fa4e44e94b81b14289afa..db8e06cc3fa40e9be39f129245fdfc7b62059a4f 100644 (file)
--- a/clunk.c
+++ b/clunk.c
@@ -1,3 +1,4 @@
+#include <aio.h>
 #include <err.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -31,15 +32,17 @@ static int const maxadvance = (10 * 3600) + (50 * 60);
 static struct tm displayed;
 
 static int statefd = -1;
+static struct aiocb stateaio;
+static char statebuf[10];
 
-static sigset_t signals_to_block;
+static sigset_t signals_to_block, timing_signals;
 
 /*
  * The fallback timer protects us from occasions when CLOCK_REALTIME
  * goes backwards so our nice absolute clock_nanosleep() end up
  * sleeping far too long.
  */
-static timer_t fallback_timer;
+static timer_t main_timer, fallback_timer;
 
 static void
 dummy_out(bool state)
@@ -68,10 +71,8 @@ libgpiod_out(bool state)
 static void (*outfn)(bool) = &dummy_out;
 
 static void
-record_tick()
+record_tick(void)
 {
-       ssize_t ret;
-       char buf[10];
 
        displayed.tm_sec += 30;
        while (displayed.tm_sec >= 60) {
@@ -83,13 +84,41 @@ record_tick()
                displayed.tm_min -= 60;
        }
        displayed.tm_hour %= 12;
-       snprintf(buf, 10, "%2d:%02d:%02d\n",
-                displayed.tm_hour, displayed.tm_min, displayed.tm_sec);
        if (statefd != -1) {
-               ret = pwrite(statefd, buf, 9, 0);
-               if (ret == -1)
+               /*
+                * The straightforward thing to do here would simply
+                * be to pwrite() to the state file.  However, we
+                * don't do that because we'd like to drop the clock
+                * GPIO line in 250 ms, and the write might take
+                * longer than that (especially since statefd is
+                * opened with O_DSYNC).  So it's time to plunge into
+                * POSIX aio.  Happily, it's pleasant enough in a
+                * simple case like this.
+                */
+               snprintf(statebuf, 10, "%2d:%02d:%02d\n", displayed.tm_hour,
+                        displayed.tm_min, displayed.tm_sec);
+               stateaio.aio_fildes = statefd;
+               stateaio.aio_buf = statebuf;
+               stateaio.aio_nbytes = 9;
+               stateaio.aio_offset = 0;
+               stateaio.aio_reqprio = 0;
+               stateaio.aio_sigevent.sigev_notify = SIGEV_NONE;
+               if (aio_write(&stateaio) == -1)
+                       err(1, "aio_write to state file");
+       }
+}
+
+static void
+record_tick_finished(void)
+{
+       struct aiocb const *stateaiop = &stateaio;
+       
+       if (statefd != -1) {
+               if (aio_suspend(&stateaiop, 1, NULL) == -1)
+                       err(1, "aio_suspend");
+               if (aio_error(&stateaio) == -1)
                        err(1, "write to state file");
-               if (ret != 9)
+               if (aio_return(&stateaio) != 9)
                        errx(1, "short write to state file");
        }
 }
@@ -108,6 +137,7 @@ pulse()
        if (errno != 0)
                err(1, "clock_nanosleep");
        (*outfn)(false);
+       record_tick_finished();
        if (sigprocmask(SIG_SETMASK, &saved_mask, NULL) != 0)
                err(1, "sigprocmask(restore)");
 }
@@ -149,19 +179,14 @@ init_libgpiod(char const *gpio_name)
                err(1, "requesting '%s'", gpio_name);
 }
 
-static void
-alrm_handler(int signo)
-{
-
-       /* Do nothing.  The mere presence of this handler is enough. */
-}
-
 static void
 init(int argc, char **argv)
 {
        struct timespec ts;
-       struct sigevent sev;
-       struct sigaction sa;
+       struct sigevent sev = {
+               .sigev_notify = SIGEV_SIGNAL,
+               .sigev_signo = SIGALRM
+       };
        char *statefile = NULL, *statestr = NULL;
        int opt;
 
@@ -170,16 +195,16 @@ init(int argc, char **argv)
            sigaddset(&signals_to_block, SIGINT) != 0 ||
            sigaddset(&signals_to_block, SIGTERM) != 0)
                err(1, "sigset");
-       sev.sigev_notify = SIGEV_SIGNAL;
-       sev.sigev_signo = SIGALRM;
+       if (sigemptyset(&timing_signals) != 0 ||
+           sigaddset(&timing_signals, SIGALRM) != 0)
+               err(1, "sigset");
+       if (sigprocmask(SIG_BLOCK, &timing_signals, NULL) == -1)
+               err(1, "sigprocmask");
+       if (timer_create(CLOCK_REALTIME, &sev, &main_timer) != 0)
+               err(1, "timer_create");
        if (timer_create(CLOCK_MONOTONIC, &sev, &fallback_timer) != 0)
                err(1, "timer_create");
-       sa.sa_handler = alrm_handler;
-       if (sigemptyset(&sa.sa_mask) != 0)
-               err(1, "sigemptyset");
-       sa.sa_flags = 0;
-       if (sigaction(SIGALRM, &sa, NULL) != 0)
-               err(1, "sigaction");
+
        if (clock_gettime(CLOCK_REALTIME, &ts) != 0)
                err(1, "clock_gettime");
        if (localtime_r(&ts.tv_sec, &displayed) == NULL)
@@ -235,15 +260,20 @@ static void
 run()
 {
        struct timespec ts;
-       const struct itimerspec its = {
+       struct itimerspec its_main = {
+               .it_interval = { .tv_sec = 0, .tv_nsec = 0}
+       };
+       const struct itimerspec its_fallback = {
                .it_value    = { .tv_sec = 35, .tv_nsec = 0},
-               .it_interval = { .tv_sec = 1,  .tv_nsec = 0},
+               .it_interval = { .tv_sec = 0,  .tv_nsec = 0},
        };
        const struct itimerspec its_disarm = {
                .it_value =  { .tv_sec = 0, .tv_nsec = 0 },
        };
        struct tm tm;
        int tick;
+       sigset_t pending_signals;
+       int signo;
 
        while (true) {
                if (clock_gettime(CLOCK_REALTIME, &ts) != 0)
@@ -268,14 +298,27 @@ run()
                /* Choose when next tick will be. */
                ts.tv_nsec = 1000000000 - pulsewidth;
                ts.tv_sec += tick - 1;
-               if (timer_settime(fallback_timer, 0, &its, NULL) != 0)
-                       err(1, "timer_settime (arm)");
-               errno = clock_nanosleep(CLOCK_REALTIME, TIMER_ABSTIME,
-                                       &ts, NULL);
-               if (errno != 0 && errno != EINTR)
-                       err(1, "clock_nanosleep");
+               its_main.it_value = ts;
+               if (timer_settime(main_timer, TIMER_ABSTIME, &its_main, NULL)
+                   != 0)
+                       err(1, "timer_settime (arm main)");
+               if (timer_settime(fallback_timer, 0, &its_fallback, NULL) != 0)
+                       err(1, "timer_settime (arm fallback)");
+               errno = sigwait(&timing_signals, &signo);
+               if (errno != 0)
+                       err(1, "sigwait");
+               if (timer_settime(main_timer, 0, &its_disarm, NULL) != 0)
+                       err(1, "timer_settime (disarm)");
                if (timer_settime(fallback_timer, 0, &its_disarm, NULL) != 0)
                        err(1, "timer_settime (disarm)");
+               /* Clear any pending signals */
+               if (sigpending(&pending_signals) == -1)
+                       err(1, "sigpending");
+               if (sigismember(&pending_signals, SIGALRM)) {
+                       errno = sigwait(&timing_signals, &signo);
+                       if (errno != 0)
+                               err(1, "sigwait");
+               }
        }
 }