From 81859b8a57567171692f334833d4bbbaf3fc6057 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Thu, 23 Sep 2021 11:45:42 -0700 Subject: [PATCH] timeout: use the monotonic clock. On a desktop or server the difference between the realtime and monotonic clocks doesn't matter much, but we've seen cases on Android where something's run under timeout(1) but killed before it's had a chance to do anything because the device went into sleep shortly afterwards and by the time it came back, the timeout had elapsed (as far as the realtime clock is concerned) but the process had only had a tiny fraction of the timeout where the system wasn't suspended. This patch also adds an xparsetimespec() function; at this point the only remaining users of xparsetime() are in lib, so maybe that should become static, at least until we have another need for it? A bigger question is whether timeout(1) needs to offer the user a choice between the two different clocks? Although monotonic is usally the right default choice on Android, and I don't have a specific counter-example to hand, I can imagine that someone might actually mean "wall clock time" rather than "cpu clock time" when setting a timeout... Luckily, timer_create() lets us trivially choose between both clocks if so. --- lib/lib.h | 1 + lib/xwrap.c | 4 ++++ scripts/make.sh | 2 +- toys/other/reboot.c | 6 +++--- toys/other/timeout.c | 29 ++++++++++++----------------- toys/posix/sleep.c | 6 +++--- 6 files changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/lib.h b/lib/lib.h index 920f0b3b..2b940600 100644 --- a/lib/lib.h +++ b/lib/lib.h @@ -187,6 +187,7 @@ char *xreadlinkat(int dir, char *name); char *xreadlink(char *name); double xstrtod(char *s); long xparsetime(char *arg, long units, long *fraction); +void xparsetimespec(char *arg, struct timespec *ts); long long xparsemillitime(char *arg); void xpidfile(char *name); void xregcomp(regex_t *preg, char *rexec, int cflags); diff --git a/lib/xwrap.c b/lib/xwrap.c index d2680cda..449e7b14 100644 --- a/lib/xwrap.c +++ b/lib/xwrap.c @@ -922,6 +922,10 @@ long long xparsemillitime(char *arg) return (l*1000LL)+ll; } +void xparsetimespec(char *arg, struct timespec *ts) +{ + ts->tv_sec = xparsetime(arg, 9, &ts->tv_nsec); +} // Compile a regular expression into a regex_t diff --git a/scripts/make.sh b/scripts/make.sh index f836e044..3caa855d 100755 --- a/scripts/make.sh +++ b/scripts/make.sh @@ -108,7 +108,7 @@ then # and skip nonexistent libraries for it. > generated/optlibs.dat - for i in util crypt m resolv selinux smack attr crypto z log iconv + for i in util crypt m resolv rt selinux smack attr crypto z log iconv do echo "int main(int argc, char *argv[]) {return 0;}" | \ ${CROSS_COMPILE}${CC} $CFLAGS $LDFLAGS -xc - -o generated/libprobe $LDASNEEDED -l$i > /dev/null 2>/dev/null && diff --git a/toys/other/reboot.c b/toys/other/reboot.c index ff8ef966..4770d1f1 100644 --- a/toys/other/reboot.c +++ b/toys/other/reboot.c @@ -29,13 +29,13 @@ GLOBALS( void reboot_main(void) { - struct timespec tv; + struct timespec ts; int types[] = {RB_AUTOBOOT, RB_HALT_SYSTEM, RB_POWER_OFF}, sigs[] = {SIGTERM, SIGUSR1, SIGUSR2}, idx; if (TT.d) { - tv.tv_sec = xparsetime(TT.d, 9, &tv.tv_nsec); - nanosleep(&tv, NULL); + xparsetimespec(TT.d, &ts); + nanosleep(&ts, NULL); } if (!FLAG(n)) sync(); diff --git a/toys/other/timeout.c b/toys/other/timeout.c index 1e125300..9f7cd1f1 100644 --- a/toys/other/timeout.c +++ b/toys/other/timeout.c @@ -33,8 +33,9 @@ GLOBALS( int nextsig; pid_t pid; - struct timeval ktv; - struct itimerval itv; + struct timespec kts; + struct itimerspec its; + timer_t timer; ) static void handler(int i) @@ -48,27 +49,21 @@ static void handler(int i) TT.k = 0; TT.nextsig = SIGKILL; xsignal(SIGALRM, handler); - TT.itv.it_value = TT.ktv; - setitimer(ITIMER_REAL, &TT.itv, (void *)toybuf); + TT.its.it_value = TT.kts; + if (timer_settime(TT.timer, 0, &TT.its, 0)) perror_exit("timer_settime"); } } -// timeval inexplicably makes up a new type for microseconds, despite timespec's -// nanoseconds field (needing to store 1000* the range) using "long". Bravo. -static void xparsetimeval(char *s, struct timeval *tv) -{ - long ll; - - tv->tv_sec = xparsetime(s, 6, &ll); - tv->tv_usec = ll; -} - void timeout_main(void) { + struct sigevent se = { .sigev_notify = SIGEV_SIGNAL, .sigev_signo = SIGALRM }; + // Use same ARGFAIL value for any remaining parsing errors toys.exitval = 125; - xparsetimeval(*toys.optargs, &TT.itv.it_value); - if (TT.k) xparsetimeval(TT.k, &TT.ktv); + xparsetimespec(*toys.optargs, &TT.its.it_value); + if (TT.k) xparsetimespec(TT.k, &TT.kts); + + if (timer_create(CLOCK_MONOTONIC, &se, &TT.timer)) perror_exit("timer"); TT.nextsig = SIGTERM; if (TT.s && -1 == (TT.nextsig = sig_to_num(TT.s))) @@ -82,7 +77,7 @@ void timeout_main(void) int status; xsignal(SIGALRM, handler); - setitimer(ITIMER_REAL, &TT.itv, (void *)toybuf); + if (timer_settime(TT.timer, 0, &TT.its, 0)) perror_exit("timer_settime"); status = xwaitpid(TT.pid); if (FLAG(preserve_status) || !toys.exitval) toys.exitval = status; diff --git a/toys/posix/sleep.c b/toys/posix/sleep.c index 846df80c..73f03fb4 100644 --- a/toys/posix/sleep.c +++ b/toys/posix/sleep.c @@ -23,8 +23,8 @@ config SLEEP void sleep_main(void) { - struct timespec tv; + struct timespec ts; - tv.tv_sec = xparsetime(*toys.optargs, 9, &tv.tv_nsec); - toys.exitval = !!nanosleep(&tv, NULL); + xparsetimespec(*toys.optargs, &ts); + toys.exitval = !!nanosleep(&ts, NULL); } -- 2.39.2