From 60933bb89b3fe9be65d1ca6fec78b715c5714f66 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Wed, 28 Feb 2018 15:07:30 +0000 Subject: [PATCH 1/3] core: timer_enter_waiting(): refactor `base` local variable We have two variables `b` and `base`. `b` is declared within limited scope; `base` is declared at the top of the function. However `base` is actually only used within a scope which is exclusive of `b`. Clarify by moving `base` inside the limited scope as well. (Also `base` doesn't need initializing any more than `b` does. The declaration of `base` is now immediately followed by a case analysis of `v->base`, which serves almost exclusively to determine the value of `base`). --- src/core/timer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/timer.c b/src/core/timer.c index 133cbb974d..1770c593f8 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -354,7 +354,6 @@ static void timer_enter_waiting(Timer *t, bool initial) { bool found_monotonic = false, found_realtime = false; bool leave_around = false; triple_timestamp ts; - usec_t base = 0; TimerValue *v; Unit *trigger; int r; @@ -372,7 +371,6 @@ static void timer_enter_waiting(Timer *t, bool initial) { t->next_elapse_monotonic_or_boottime = t->next_elapse_realtime = 0; LIST_FOREACH(value, v, t->values) { - if (v->disabled) continue; @@ -397,7 +395,8 @@ static void timer_enter_waiting(Timer *t, bool initial) { found_realtime = true; - } else { + } else { + usec_t base; switch (v->base) { From 9ea9faff7866ac7aaa2dc529fb92c5cdd643fc04 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Wed, 28 Feb 2018 15:34:16 +0000 Subject: [PATCH 2/3] core: let OnCalendar= timer units expire during suspend (#8231) On timejumps, including suspend, timer_time_change() calls for a re-calculation of the next elapse. Sadly I'm not quite sure what the intended effect of this was! Because it was not managing to fire OnCalendar= timers which fired during the suspend... unless the timer had already fired once before. Reported, entirely correctly as far as I can see, on stackexchange: https://unix.stackexchange.com/questions/351829/systemd-timer-that-expired-while-suspended /* If we know the last time this was * triggered, schedule the job based relative - * to that. If we don't just start from - * now. */ + * to that. If we don't, just start from + * the activation time. */ The same code is called for both the initial calculation and this re-calculation. If we're _not_ already active, then this is before the activation time has been recorded in the unit, so just use the current time as before. The new code is mechanically adapted from the same logic for `OnActiveSec=` (case TIMER_ACTIVE in the code which follows). Tested with `date --set`. Motivations: * Rotate monitoring data from Atop into files which are named per-day. Fedora currently implements this with a cron job that runs at midnight, but that didn't handle suspend correctly either. * unbound-anchor.timer on Fedora, is used to update DNSSEC "root trust anchor" daily, before the TTL expires. It uses OnCalendar=daily AccuracySec=24h. Which is a bit suspect because the TTL is 2 days, but I think it has the right general idea. None of the other timer settings are correct, because they would not account for time spent in suspend. Unless you set WakeSystem (this feature is currently undocumented). * So in general, we can expect to see people using OnCalendar= for the same cases as cron.daily and cron.monthly. Which use anacron to keep track of jobs which should be run even if the system was down at the time. Timers which are configured to run more frequently than that, are unlikely to mind if they get run slightly more often that the writer realized, relative to the amount of time the system was really running. * From the user report above: "I only want to use remind to show a desktop notification, it seems excessive to wake up the computer for that. Also, I would like to get the reminder first thing in the morning, so the OnActiveSec doesn't help with that." --- src/core/timer.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/core/timer.c b/src/core/timer.c index 1770c593f8..e0f39584ce 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -379,10 +379,17 @@ static void timer_enter_waiting(Timer *t, bool initial) { /* If we know the last time this was * triggered, schedule the job based relative - * to that. If we don't just start from - * now. */ + * to that. If we don't, just start from + * the activation time. */ - b = t->last_trigger.realtime > 0 ? t->last_trigger.realtime : ts.realtime; + if (t->last_trigger.realtime > 0) + b = t->last_trigger.realtime; + else { + if (state_translation_table[t->state] == UNIT_ACTIVE) + b = UNIT(t)->inactive_exit_timestamp.realtime; + else + b = ts.realtime; + } r = calendar_spec_next_usec(v->calendar_spec, b, &v->next_elapse); if (r < 0) From 13f512d324e9e9be7286391347a185b716f0707c Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Wed, 28 Feb 2018 16:03:43 +0000 Subject: [PATCH 3/3] core: don't freeze OnCalendar= timer units when the clock goes back a lot E.g. if you have a monthly event and you set the computer clock back one year, we can allow the next 12 monthly events to happen naturally. In fact we already do this when you start a Persistent=yes timer, we just need to apply the same logic when it's running and we notice the system clock being set backwards. --- src/core/timer.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/core/timer.c b/src/core/timer.c index e0f39584ce..ddb9c82b87 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -813,12 +813,21 @@ static void timer_reset_failed(Unit *u) { static void timer_time_change(Unit *u) { Timer *t = TIMER(u); + usec_t ts; assert(u); if (t->state != TIMER_WAITING) return; + /* If we appear to have triggered in the future, the system clock must + * have been set backwards. So let's rewind our own clock and allow + * the future trigger(s) to happen again :). Exactly the same as when + * you start a timer unit with Persistent=yes. */ + ts = now(CLOCK_REALTIME); + if (t->last_trigger.realtime > ts) + t->last_trigger.realtime = ts; + log_unit_debug(u, "Time change, recalculating next elapse."); timer_enter_waiting(t, false); }