From: Ben Harris Date: Fri, 23 Nov 2018 18:45:52 +0000 (+0000) Subject: Use two timers and sigwait rather than looking for EINTR. X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~bjharris/git?a=commitdiff_plain;h=973a794918444a2c64a1447e8222dc74abee87d1;p=clunk.git 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 1879e70..db8e06c 100644 --- a/clunk.c +++ b/clunk.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -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"); + } } }