From 40c6b5143b2a3149d74e00a76332fc6f4fe058e7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 14:03:23 +0100 Subject: [PATCH 01/16] timesyncd: use structured initialization more --- src/timesync/timesyncd-manager.c | 49 ++++++++++++++++++-------------- src/timesync/timesyncd-server.c | 21 ++++++++------ 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 918da195d8..c14878bd21 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -244,7 +244,7 @@ static int manager_clock_watch_setup(Manager *m) { } static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { - struct timex tmx = {}; + struct timex tmx; int r; assert(m); @@ -254,21 +254,22 @@ static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { * clock to the NTP time, larger deltas are just directly set. */ if (fabs(offset) < NTP_MAX_ADJUST) { - tmx.modes = ADJ_STATUS | ADJ_NANO | ADJ_OFFSET | ADJ_TIMECONST | ADJ_MAXERROR | ADJ_ESTERROR; - tmx.status = STA_PLL; - tmx.offset = offset * NSEC_PER_SEC; - tmx.constant = log2i(m->poll_interval_usec / USEC_PER_SEC) - 4; - tmx.maxerror = 0; - tmx.esterror = 0; + tmx = (struct timex) { + .modes = ADJ_STATUS | ADJ_NANO | ADJ_OFFSET | ADJ_TIMECONST | ADJ_MAXERROR | ADJ_ESTERROR, + .status = STA_PLL, + .offset = offset * NSEC_PER_SEC, + .constant = log2i(m->poll_interval_usec / USEC_PER_SEC) - 4, + }; + log_debug(" adjust (slew): %+.3f sec", offset); } else { - tmx.modes = ADJ_STATUS | ADJ_NANO | ADJ_SETOFFSET | ADJ_MAXERROR | ADJ_ESTERROR; + tmx = (struct timex) { + .modes = ADJ_STATUS | ADJ_NANO | ADJ_SETOFFSET | ADJ_MAXERROR | ADJ_ESTERROR, - /* ADJ_NANO uses nanoseconds in the microseconds field */ - tmx.time.tv_sec = (long)offset; - tmx.time.tv_usec = (offset - tmx.time.tv_sec) * NSEC_PER_SEC; - tmx.maxerror = 0; - tmx.esterror = 0; + /* ADJ_NANO uses nanoseconds in the microseconds field */ + .time.tv_sec = (long)offset, + .time.tv_usec = (offset - (double) (long) offset) * NSEC_PER_SEC, + }; /* the kernel expects -0.3s as {-1, 7000.000.000} */ if (tmx.time.tv_usec < 0) { @@ -1098,21 +1099,27 @@ int manager_new(Manager **ret) { assert(ret); - m = new0(Manager, 1); + m = new(Manager, 1); if (!m) return -ENOMEM; - m->root_distance_max_usec = NTP_ROOT_DISTANCE_MAX_USEC; - m->poll_interval_min_usec = NTP_POLL_INTERVAL_MIN_USEC; - m->poll_interval_max_usec = NTP_POLL_INTERVAL_MAX_USEC; + *m = (Manager) { + .root_distance_max_usec = NTP_ROOT_DISTANCE_MAX_USEC, + .poll_interval_min_usec = NTP_POLL_INTERVAL_MIN_USEC, + .poll_interval_max_usec = NTP_POLL_INTERVAL_MAX_USEC, - m->connection_retry_usec = DEFAULT_CONNECTION_RETRY_USEC; + .connection_retry_usec = DEFAULT_CONNECTION_RETRY_USEC, - m->server_socket = m->clock_watch_fd = -1; + .server_socket = -1, + .clock_watch_fd = -1, - m->ratelimit = (RateLimit) { RATELIMIT_INTERVAL_USEC, RATELIMIT_BURST }; + .ratelimit = (RateLimit) { + RATELIMIT_INTERVAL_USEC, + RATELIMIT_BURST + }, - m->save_time_interval_usec = DEFAULT_SAVE_TIME_INTERVAL_USEC; + .save_time_interval_usec = DEFAULT_SAVE_TIME_INTERVAL_USEC, + }; r = sd_event_default(&m->event); if (r < 0) diff --git a/src/timesync/timesyncd-server.c b/src/timesync/timesyncd-server.c index 79dfd47266..dd16891734 100644 --- a/src/timesync/timesyncd-server.c +++ b/src/timesync/timesyncd-server.c @@ -16,16 +16,19 @@ int server_address_new( assert(socklen >= offsetof(struct sockaddr, sa_data)); assert(socklen <= sizeof(union sockaddr_union)); - a = new0(ServerAddress, 1); + a = new(ServerAddress, 1); if (!a) return -ENOMEM; + *a = (ServerAddress) { + .name = n, + .socklen = socklen, + }; + memcpy(&a->sockaddr, sockaddr, socklen); - a->socklen = socklen; LIST_FIND_TAIL(addresses, n->addresses, tail); LIST_INSERT_AFTER(addresses, n->addresses, tail, a); - a->name = n; if (ret) *ret = a; @@ -58,12 +61,16 @@ int server_name_new( assert(m); assert(string); - n = new0(ServerName, 1); + n = new(ServerName, 1); if (!n) return -ENOMEM; - n->type = type; - n->string = strdup(string); + *n = (ServerName) { + .manager = m, + .type = type, + .string = strdup(string), + }; + if (!n->string) { free(n); return -ENOMEM; @@ -81,8 +88,6 @@ int server_name_new( } else assert_not_reached(); - n->manager = m; - if (type != SERVER_FALLBACK && m->current_server_name && m->current_server_name->type == SERVER_FALLBACK) From aabda4f3e0f20b4ab76affd2d1ca2084cc2cea3e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:55:24 +0100 Subject: [PATCH 02/16] timesyncd: get rid of unnecessary 'r' assignments --- src/timesync/timesyncd-manager.c | 3 +-- src/timesync/timesyncd.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index c14878bd21..707ebe487d 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -301,8 +301,7 @@ static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { break; } - r = clock_adjtime(CLOCK_REALTIME, &tmx); - if (r < 0) + if (clock_adjtime(CLOCK_REALTIME, &tmx) < 0) return -errno; r = manager_save_time_and_rearm(m); diff --git a/src/timesync/timesyncd.c b/src/timesync/timesyncd.c index 471a63689e..bc3e6ee91b 100644 --- a/src/timesync/timesyncd.c +++ b/src/timesync/timesyncd.c @@ -40,8 +40,7 @@ static int load_clock_timestamp(uid_t uid, gid_t gid) { usec_t stamp; /* check if the recorded time is later than the compiled-in one */ - r = fstat(fd, &st); - if (r >= 0) { + if (fstat(fd, &st) >= 0) { stamp = timespec_load(&st.st_mtim); if (stamp > min) min = stamp; From c696878ac03f229d69a9045a22fb19a5617034a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 14:06:26 +0100 Subject: [PATCH 03/16] timesyncd: cast some function invocations to void --- src/timesync/timesyncd-manager.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 707ebe487d..262865d58e 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -626,10 +626,11 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re m->good = true; - server_address_pretty(m->current_server_address, &pretty); + (void) server_address_pretty(m->current_server_address, &pretty); + /* "Initial", as further successful syncs will not be logged. */ log_info("Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); - sd_notifyf(false, "STATUS=Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); + (void) sd_notifyf(false, "STATUS=Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); } r = manager_arm_timer(m, m->poll_interval_usec); @@ -693,7 +694,7 @@ static int manager_begin(Manager *m) { server_address_pretty(m->current_server_address, &pretty); log_debug("Connecting to time server %s (%s).", strna(pretty), m->current_server_name->string); - sd_notifyf(false, "STATUS=Connecting to time server %s (%s).", strna(pretty), m->current_server_name->string); + (void) sd_notifyf(false, "STATUS=Connecting to time server %s (%s).", strna(pretty), m->current_server_name->string); r = manager_clock_watch_setup(m); if (r < 0) @@ -915,7 +916,7 @@ void manager_disconnect(Manager *m) { m->event_timeout = sd_event_source_unref(m->event_timeout); - sd_notify(false, "STATUS=Idle."); + (void) sd_notify(false, "STATUS=Idle."); } void manager_flush_server_names(Manager *m, ServerType t) { From 853efc39eab4f570fb892bf370d0794d3ac1a338 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 14:06:47 +0100 Subject: [PATCH 04/16] timesyncd: make a constant structure actually static/const --- src/timesync/timesyncd-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 262865d58e..6107531a3b 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -819,7 +819,7 @@ int manager_connect(Manager *m) { if (m->current_server_address && m->current_server_address->addresses_next) manager_set_server_address(m, m->current_server_address->addresses_next); else { - struct addrinfo hints = { + static const struct addrinfo hints = { .ai_flags = AI_NUMERICSERV|AI_ADDRCONFIG, .ai_socktype = SOCK_DGRAM, }; From 0fd08026f83f6e8c39eb5025470f84b6cc5f2742 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 14:07:22 +0100 Subject: [PATCH 05/16] timesyncd: add comment what bool arg is --- src/timesync/timesyncd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/timesync/timesyncd.c b/src/timesync/timesyncd.c index bc3e6ee91b..98766a9ce7 100644 --- a/src/timesync/timesyncd.c +++ b/src/timesync/timesyncd.c @@ -63,7 +63,7 @@ static int load_clock_timestamp(uid_t uid, gid_t gid) { } /* create stamp file with the compiled-in date */ - r = touch_file(CLOCK_FILE, false, min, uid, gid, 0644); + r = touch_file(CLOCK_FILE, /* parents= */ false, min, uid, gid, 0644); if (r < 0) log_debug_errno(r, "Failed to create %s, ignoring: %m", CLOCK_FILE); } From 9a8b43021b7e6712bade0d57bbe0d03e96ce65b3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 15:27:20 +0100 Subject: [PATCH 06/16] timesyncd: rebreak some comments --- src/timesync/timesyncd-manager.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 6107531a3b..60be465caa 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -249,10 +249,8 @@ static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { assert(m); - /* - * For small deltas, tell the kernel to gradually adjust the system - * clock to the NTP time, larger deltas are just directly set. - */ + /* For small deltas, tell the kernel to gradually adjust the system clock to the NTP time, larger + * deltas are just directly set. */ if (fabs(offset) < NTP_MAX_ADJUST) { tmx = (struct timex) { .modes = ADJ_STATUS | ADJ_NANO | ADJ_OFFSET | ADJ_TIMECONST | ADJ_MAXERROR | ADJ_ESTERROR, @@ -281,14 +279,11 @@ static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { log_debug(" adjust (jump): %+.3f sec", offset); } - /* - * An unset STA_UNSYNC will enable the kernel's 11-minute mode, - * which syncs the system time periodically to the RTC. + /* An unset STA_UNSYNC will enable the kernel's 11-minute mode, which syncs the system time + * periodically to the RTC. * - * In case the RTC runs in local time, never touch the RTC, - * we have no way to properly handle daylight saving changes and - * mobile devices moving between time zones. - */ + * In case the RTC runs in local time, never touch the RTC, we have no way to properly handle + * daylight saving changes and mobile devices moving between time zones. */ if (m->rtc_local_time) tmx.status |= STA_UNSYNC; From 5ba674cca59bed9d58eaf06271c8efe5f17fe4cb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:55:29 +0100 Subject: [PATCH 07/16] timesyncd: merge some variable declarations --- src/timesync/timesyncd-manager.c | 7 ++----- src/timesync/timesyncd.c | 3 +-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 60be465caa..4781fcadc1 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -425,12 +425,9 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re struct cmsghdr *cmsg; struct timespec *recv_time = NULL; ssize_t len; - double origin, receive, trans, dest; - double delay, offset; - double root_distance; + double origin, receive, trans, dest, delay, offset, root_distance; bool spike; - int leap_sec; - int r; + int leap_sec, r; assert(source); assert(m); diff --git a/src/timesync/timesyncd.c b/src/timesync/timesyncd.c index 98766a9ce7..999d1d3852 100644 --- a/src/timesync/timesyncd.c +++ b/src/timesync/timesyncd.c @@ -22,9 +22,8 @@ #include "user-util.h" static int load_clock_timestamp(uid_t uid, gid_t gid) { + usec_t min = TIME_EPOCH * USEC_PER_SEC, ct; _cleanup_close_ int fd = -1; - usec_t min = TIME_EPOCH * USEC_PER_SEC; - usec_t ct; int r; /* Let's try to make sure that the clock is always From adc07c2bf5338b709c0771e51b78addd5006e54f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 15:52:06 +0100 Subject: [PATCH 08/16] timesyncd: use CMSG_FIND_DATA() macro where appropriate --- src/timesync/timesyncd-manager.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 4781fcadc1..302f09125f 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -422,7 +422,6 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re .msg_name = &server_addr, .msg_namelen = sizeof(server_addr), }; - struct cmsghdr *cmsg; struct timespec *recv_time = NULL; ssize_t len; double origin, receive, trans, dest, delay, offset, root_distance; @@ -458,21 +457,9 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re return 0; } - CMSG_FOREACH(cmsg, &msghdr) { - if (cmsg->cmsg_level != SOL_SOCKET) - continue; - - switch (cmsg->cmsg_type) { - case SCM_TIMESTAMPNS: - assert(cmsg->cmsg_len == CMSG_LEN(sizeof(struct timespec))); - - recv_time = (struct timespec *) CMSG_DATA(cmsg); - break; - } - } + recv_time = CMSG_FIND_DATA(&msghdr, SOL_SOCKET, SCM_TIMESTAMPNS, struct timespec); if (!recv_time) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Invalid packet timestamp."); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Packet timestamp missing."); if (!m->pending) { log_debug("Unexpected reply. Ignoring."); From 7cb08ea80f66dbaadfa0350cb09adf155449bcc6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 15:53:25 +0100 Subject: [PATCH 09/16] timesyncd: move stuff that is not about setting the clock out of manager_adjust_clock() Let's make sure manager_adjust_clock() is purely about setting the clock, and nothing else. Let's clean up logging this way. manager_adjust_clock() now won#t log about errors, but the caller can safely do that, and do with the right log message string. --- src/timesync/timesyncd-manager.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 302f09125f..3ab08a0fd6 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -245,7 +245,6 @@ static int manager_clock_watch_setup(Manager *m) { static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { struct timex tmx; - int r; assert(m); @@ -299,13 +298,6 @@ static int manager_adjust_clock(Manager *m, double offset, int leap_sec) { if (clock_adjtime(CLOCK_REALTIME, &tmx) < 0) return -errno; - r = manager_save_time_and_rearm(m); - if (r < 0) - return r; - - /* If touch fails, there isn't much we can do. Maybe it'll work next time. */ - (void) touch("/run/systemd/timesync/synchronized"); - m->drift_freq = tmx.freq; log_debug(" status : %04i %s\n" @@ -580,6 +572,13 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re r = manager_adjust_clock(m, offset, leap_sec); if (r < 0) log_error_errno(r, "Failed to call clock_adjtime(): %m"); + + r = manager_save_time_and_rearm(m); + if (r < 0) + return r; + + /* If touch fails, there isn't much we can do. Maybe it'll work next time. */ + (void) touch("/run/systemd/timesync/synchronized"); } /* Save NTP response */ From ec75e8e07a0ad972e0c40e0a187e15a8d4fb3d66 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:28:38 +0100 Subject: [PATCH 10/16] sd-event: add a single implementation of an event source that runs on clock changes We basically had the same code in three places. Let's unify it in a common helper function. event_add_time_change() might be something we should add to the official sd-event API sooner or later, given its general usefulness. --- src/core/manager.c | 13 ++-------- src/core/manager.h | 1 - src/libsystemd/sd-event/event-util.c | 39 ++++++++++++++++++++++++++++ src/libsystemd/sd-event/event-util.h | 2 ++ src/resolve/resolved-manager.c | 15 ++--------- src/timesync/timesyncd-manager.c | 14 +++------- src/timesync/timesyncd-manager.h | 1 - 7 files changed, 49 insertions(+), 36 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 4c59506ccb..a4ff2c8355 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -39,6 +39,7 @@ #include "dirent-util.h" #include "env-util.h" #include "escape.h" +#include "event-util.h" #include "exec-util.h" #include "execute.h" #include "exit-status.h" @@ -406,13 +407,8 @@ static int manager_setup_time_change(Manager *m) { return 0; m->time_change_event_source = sd_event_source_disable_unref(m->time_change_event_source); - m->time_change_fd = safe_close(m->time_change_fd); - m->time_change_fd = time_change_fd(); - if (m->time_change_fd < 0) - return log_error_errno(m->time_change_fd, "Failed to create timer change timer fd: %m"); - - r = sd_event_add_io(m->event, &m->time_change_event_source, m->time_change_fd, EPOLLIN, manager_dispatch_time_change_fd, m); + r = event_add_time_change(m->event, &m->time_change_event_source, manager_dispatch_time_change_fd, m); if (r < 0) return log_error_errno(r, "Failed to create time change event source: %m"); @@ -421,8 +417,6 @@ static int manager_setup_time_change(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to set priority of time change event sources: %m"); - (void) sd_event_source_set_description(m->time_change_event_source, "manager-time-change"); - log_debug("Set up TFD_TIMER_CANCEL_ON_SET timerfd."); return 0; @@ -820,7 +814,6 @@ int manager_new(UnitFileScope scope, ManagerTestRunFlags test_run_flags, Manager .notify_fd = -1, .cgroups_agent_fd = -1, .signal_fd = -1, - .time_change_fd = -1, .user_lookup_fds = { -1, -1 }, .private_listen_fd = -1, .dev_autofs_fd = -1, @@ -1509,7 +1502,6 @@ Manager* manager_free(Manager *m) { safe_close(m->signal_fd); safe_close(m->notify_fd); safe_close(m->cgroups_agent_fd); - safe_close(m->time_change_fd); safe_close_pair(m->user_lookup_fds); manager_close_ask_password(m); @@ -2913,7 +2905,6 @@ static int manager_dispatch_time_change_fd(sd_event_source *source, int fd, uint Unit *u; assert(m); - assert(m->time_change_fd == fd); log_struct(LOG_DEBUG, "MESSAGE_ID=" SD_MESSAGE_TIME_CHANGE_STR, diff --git a/src/core/manager.h b/src/core/manager.h index e5d988b745..f9096cf348 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -226,7 +226,6 @@ struct Manager { sd_event_source *sigchld_event_source; - int time_change_fd; sd_event_source *time_change_event_source; sd_event_source *timezone_change_event_source; diff --git a/src/libsystemd/sd-event/event-util.c b/src/libsystemd/sd-event/event-util.c index 0e53406a94..4d0d780741 100644 --- a/src/libsystemd/sd-event/event-util.c +++ b/src/libsystemd/sd-event/event-util.c @@ -4,6 +4,7 @@ #include "event-source.h" #include "event-util.h" +#include "fd-util.h" #include "log.h" #include "string-util.h" @@ -121,3 +122,41 @@ int event_source_is_enabled(sd_event_source *s) { return sd_event_source_get_enabled(s, NULL); } + +int event_add_time_change(sd_event *e, sd_event_source **ret, sd_event_io_handler_t callback, void *userdata) { + _cleanup_(sd_event_source_disable_unrefp) sd_event_source *s = NULL; + _cleanup_close_ int fd = -1; + int r; + + assert(e); + + /* Allocates an IO event source that gets woken up whenever the clock changes. Needs to be recreated on each event */ + + fd = time_change_fd(); + if (fd < 0) + return fd; + + r = sd_event_add_io(e, &s, fd, EPOLLIN, callback, userdata); + if (r < 0) + return r; + + r = sd_event_source_set_io_fd_own(s, true); + if (r < 0) + return r; + + TAKE_FD(fd); + + r = sd_event_source_set_description(s, "time-change"); + if (r < 0) + return r; + + if (ret) + *ret = TAKE_PTR(s); + else { + r = sd_event_source_set_floating(s, true); + if (r < 0) + return r; + } + + return 0; +} diff --git a/src/libsystemd/sd-event/event-util.h b/src/libsystemd/sd-event/event-util.h index 64a4199244..abd043bdcc 100644 --- a/src/libsystemd/sd-event/event-util.h +++ b/src/libsystemd/sd-event/event-util.h @@ -29,3 +29,5 @@ int event_reset_time_relative( bool force_reset); int event_source_disable(sd_event_source *s); int event_source_is_enabled(sd_event_source *s); + +int event_add_time_change(sd_event *e, sd_event_source **ret, sd_event_io_handler_t callback, void *userdata); diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 615431ebb7..221d0afbdb 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -13,6 +13,7 @@ #include "bus-polkit.h" #include "dirent-util.h" #include "dns-domain.h" +#include "event-util.h" #include "fd-util.h" #include "fileio.h" #include "hostname-util.h" @@ -338,28 +339,16 @@ static int on_clock_change(sd_event_source *source, int fd, uint32_t revents, vo } static int manager_clock_change_listen(Manager *m) { - _cleanup_close_ int fd = -1; int r; assert(m); m->clock_change_event_source = sd_event_source_disable_unref(m->clock_change_event_source); - fd = time_change_fd(); - if (fd < 0) - return log_error_errno(fd, "Failed to allocate clock change timer fd: %m"); - - r = sd_event_add_io(m->event, &m->clock_change_event_source, fd, EPOLLIN, on_clock_change, m); + r = event_add_time_change(m->event, &m->clock_change_event_source, on_clock_change, m); if (r < 0) return log_error_errno(r, "Failed to create clock change event source: %m"); - r = sd_event_source_set_io_fd_own(m->clock_change_event_source, true); - if (r < 0) - return log_error_errno(r, "Failed to pass ownership of clock fd to event source: %m"); - TAKE_FD(fd); - - (void) sd_event_source_set_description(m->clock_change_event_source, "clock-change"); - return 0; } diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 3ab08a0fd6..193d13742b 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -14,6 +14,7 @@ #include "alloc-util.h" #include "dns-domain.h" +#include "event-util.h" #include "fd-util.h" #include "format-util.h" #include "fs-util.h" @@ -229,14 +230,9 @@ static int manager_clock_watch_setup(Manager *m) { assert(m); - m->event_clock_watch = sd_event_source_unref(m->event_clock_watch); - safe_close(m->clock_watch_fd); + m->event_clock_watch = sd_event_source_disable_unref(m->event_clock_watch); - m->clock_watch_fd = time_change_fd(); - if (m->clock_watch_fd < 0) - return log_error_errno(m->clock_watch_fd, "Failed to create timerfd: %m"); - - r = sd_event_add_io(m->event, &m->event_clock_watch, m->clock_watch_fd, EPOLLIN, manager_clock_watch, m); + r = event_add_time_change(m->event, &m->event_clock_watch, manager_clock_watch, m); if (r < 0) return log_error_errno(r, "Failed to create clock watch event source: %m"); @@ -889,8 +885,7 @@ void manager_disconnect(Manager *m) { manager_listen_stop(m); - m->event_clock_watch = sd_event_source_unref(m->event_clock_watch); - m->clock_watch_fd = safe_close(m->clock_watch_fd); + m->event_clock_watch = sd_event_source_disable_unref(m->event_clock_watch); m->event_timeout = sd_event_source_unref(m->event_timeout); @@ -1089,7 +1084,6 @@ int manager_new(Manager **ret) { .connection_retry_usec = DEFAULT_CONNECTION_RETRY_USEC, .server_socket = -1, - .clock_watch_fd = -1, .ratelimit = (RateLimit) { RATELIMIT_INTERVAL_USEC, diff --git a/src/timesync/timesyncd-manager.h b/src/timesync/timesyncd-manager.h index aceb0098ce..5368dba98c 100644 --- a/src/timesync/timesyncd-manager.h +++ b/src/timesync/timesyncd-manager.h @@ -92,7 +92,6 @@ struct Manager { /* watch for time changes */ sd_event_source *event_clock_watch; - int clock_watch_fd; /* Retry connections */ sd_event_source *event_retry; From 20c1a21066a35b4776eca5a6e25c3f8aa50a0b84 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:30:26 +0100 Subject: [PATCH 11/16] timesyncd: don't abort packet handling if we can't save the timestamp to disk This should not be a fatal error, and it isn't in the other place we call this. --- src/timesync/timesyncd-manager.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 193d13742b..b491db23bc 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -569,9 +569,7 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re if (r < 0) log_error_errno(r, "Failed to call clock_adjtime(): %m"); - r = manager_save_time_and_rearm(m); - if (r < 0) - return r; + (void) manager_save_time_and_rearm(m); /* If touch fails, there isn't much we can do. Maybe it'll work next time. */ (void) touch("/run/systemd/timesync/synchronized"); From 87057128e65c6b76c0eaf5f7f7b701831598c473 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:57:56 +0100 Subject: [PATCH 12/16] =?UTF-8?q?timesyncd:=20rename=20bool=20'good'=20?= =?UTF-8?q?=E2=86=92=20'talking'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit THe name is just too generic. Let's pick a name that makes clear that this is about successful communication. --- src/timesync/timesyncd-manager.c | 6 +++--- src/timesync/timesyncd-manager.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index b491db23bc..03363640b3 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -593,10 +593,10 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re "NTPMessage", NULL); - if (!m->good) { + if (!m->talking) { _cleanup_free_ char *pretty = NULL; - m->good = true; + m->talking = true; (void) server_address_pretty(m->current_server_address, &pretty); @@ -659,7 +659,7 @@ static int manager_begin(Manager *m) { assert_return(m->current_server_name, -EHOSTUNREACH); assert_return(m->current_server_address, -EHOSTUNREACH); - m->good = false; + m->talking = false; m->missed_replies = NTP_MAX_MISSED_REPLIES; if (m->poll_interval_usec == 0) m->poll_interval_usec = m->poll_interval_min_usec; diff --git a/src/timesync/timesyncd-manager.h b/src/timesync/timesyncd-manager.h index 5368dba98c..6244e07592 100644 --- a/src/timesync/timesyncd-manager.h +++ b/src/timesync/timesyncd-manager.h @@ -61,7 +61,7 @@ struct Manager { int missed_replies; uint64_t packet_count; sd_event_source *event_timeout; - bool good; + bool talking; /* last sent packet */ struct timespec trans_time_mon; From 1f129b7fb795251cdc7f2ee763a435a7cd531a06 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:50:59 +0100 Subject: [PATCH 13/16] timesyncd: when updating timestamp file, use best time we know, instead of system clock We adjust the system clock slowly after all, even if we know a more accurate time. Use the accurate time for the timestamp. --- src/timesync/timesyncd-manager.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 03363640b3..4895904e74 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -29,6 +29,7 @@ #include "time-util.h" #include "timesyncd-conf.h" #include "timesyncd-manager.h" +#include "user-util.h" #include "util.h" #ifndef ADJ_SETOFFSET @@ -60,7 +61,7 @@ static int manager_arm_timer(Manager *m, usec_t next); static int manager_clock_watch_setup(Manager *m); static int manager_listen_setup(Manager *m); static void manager_listen_stop(Manager *m); -static int manager_save_time_and_rearm(Manager *m); +static int manager_save_time_and_rearm(Manager *m, usec_t t); static double ntp_ts_short_to_d(const struct ntp_ts_short *ts) { return be16toh(ts->sec) + (be16toh(ts->frac) / 65536.0); @@ -411,6 +412,7 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re .msg_namelen = sizeof(server_addr), }; struct timespec *recv_time = NULL; + triple_timestamp dts; ssize_t len; double origin, receive, trans, dest, delay, offset, root_distance; bool spike; @@ -564,12 +566,18 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re m->samples_jitter, spike ? " spike" : "", m->poll_interval_usec / USEC_PER_SEC); + /* Get current monotonic/realtime clocks immediately before adjusting the latter */ + triple_timestamp_get(&dts); + if (!spike) { + /* Fix up our idea of the time. */ + dts.realtime = (usec_t) (dts.realtime + offset * USEC_PER_SEC); + r = manager_adjust_clock(m, offset, leap_sec); if (r < 0) log_error_errno(r, "Failed to call clock_adjtime(): %m"); - (void) manager_save_time_and_rearm(m); + (void) manager_save_time_and_rearm(m, dts.realtime); /* If touch fails, there isn't much we can do. Maybe it'll work next time. */ (void) touch("/run/systemd/timesync/synchronized"); @@ -1124,7 +1132,7 @@ static int manager_save_time_handler(sd_event_source *s, uint64_t usec, void *us assert(m); - (void) manager_save_time_and_rearm(m); + (void) manager_save_time_and_rearm(m, USEC_INFINITY); return 0; } @@ -1152,12 +1160,16 @@ int manager_setup_save_time_event(Manager *m) { return 0; } -static int manager_save_time_and_rearm(Manager *m) { +static int manager_save_time_and_rearm(Manager *m, usec_t t) { int r; assert(m); - r = touch(CLOCK_FILE); + /* Updates the timestamp file to the specified time. If 't' is USEC_INFINITY uses the current system + * clock, but otherwise uses the specified timestamp. Note that whenever we acquire an NTP sync the + * specified timestamp value might be more accurate than the system clock, since the latter is + * subject to slow adjustments. */ + r = touch_file(CLOCK_FILE, false, t, UID_INVALID, GID_INVALID, MODE_INVALID); if (r < 0) log_debug_errno(r, "Failed to update " CLOCK_FILE ", ignoring: %m"); From 1aa7ca224114a4280a51dfe57f0af5bc29ced65b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:53:46 +0100 Subject: [PATCH 14/16] timesyncd: add debug logging in case we can't touch /run/systemd/timesync/synchronized --- src/timesync/timesyncd-manager.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 4895904e74..ddb4927960 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -580,7 +580,9 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re (void) manager_save_time_and_rearm(m, dts.realtime); /* If touch fails, there isn't much we can do. Maybe it'll work next time. */ - (void) touch("/run/systemd/timesync/synchronized"); + r = touch("/run/systemd/timesync/synchronized"); + if (r < 0) + log_debug_errno(r, "Failed to touch /run/systemd/timesync/synchronized, ignoring: %m"); } /* Save NTP response */ From b016e77efd5ecf65027942420abd9478c2cd65a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:52:06 +0100 Subject: [PATCH 15/16] timesyncd: generate a structure log message the first time we set the clock correctly Usecase: later on we can use this to retroactively adjust log output in journalctl or similar on systems lacking an RTC: we just have to search for this sructured log message that indicates the first sync point and can then retroactively adjust the incorrect timestamps collected before that. --- catalog/systemd.catalog.in | 8 ++++++++ src/systemd/sd-messages.h | 3 +++ src/timesync/timesyncd-manager.c | 18 ++++++++++++++++++ src/timesync/timesyncd-manager.h | 3 +++ 4 files changed, 32 insertions(+) diff --git a/catalog/systemd.catalog.in b/catalog/systemd.catalog.in index a5d7dc6f78..a3f05c0698 100644 --- a/catalog/systemd.catalog.in +++ b/catalog/systemd.catalog.in @@ -526,3 +526,11 @@ be updated to operate in a hotplug fashion without depending on systemd-udev-settle.service: @OFFENDING_UNITS@ + +-- 7c8a41f37b764941a0e1780b1be2f037 +Subject: Initial clock synchronization +Defined-By: systemd +Support: %SUPPORT_URL% + +For the first time during the current boot an NTP synchronization has been +acquired and the local system clock adjustment has been initiated. diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index b9445cf0a9..f3cca758a0 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -200,6 +200,9 @@ _SD_BEGIN_DECLARATIONS; #define SD_MESSAGE_SYSTEMD_UDEV_SETTLE_DEPRECATED_STR \ SD_ID128_MAKE_STR(1c,04,54,c1,bd,22,41,e0,ac,6f,ef,b4,bc,63,14,33) +#define SD_MESSAGE_TIME_SYNC SD_ID128_MAKE(7c,8a,41,f3,7b,76,49,41,a0,e1,78,0b,1b,e2,f0,37) +#define SD_MESSAGE_TIME_SYNC_STR SD_ID128_MAKE_STR(7c,8a,41,f3,7b,76,49,41,a0,e1,78,0b,1b,e2,f0,37) + _SD_END_DECLARATIONS; #endif diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index ddb4927960..ba7e52f6b3 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -11,6 +11,7 @@ #include #include "sd-daemon.h" +#include "sd-messages.h" #include "alloc-util.h" #include "dns-domain.h" @@ -615,6 +616,17 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re (void) sd_notifyf(false, "STATUS=Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); } + if (!spike && !m->synchronized) { + m->synchronized = true; + + log_struct(LOG_INFO, + LOG_MESSAGE("Initial clock synchronization to %s.", FORMAT_TIMESTAMP_STYLE(dts.realtime, TIMESTAMP_US)), + "MESSAGE_ID=" SD_MESSAGE_TIME_SYNC_STR, + "MONOTONIC_USEC=" USEC_FMT, dts.monotonic, + "REALTIME_USEC=" USEC_FMT, dts.realtime, + "BOOTIME_USEC=" USEC_FMT, dts.boottime); + } + r = manager_arm_timer(m, m->poll_interval_usec); if (r < 0) return log_error_errno(r, "Failed to rearm timer: %m"); @@ -1110,6 +1122,12 @@ int manager_new(Manager **ret) { (void) sd_event_set_watchdog(m->event, true); + /* Load previous synchronization state */ + r = access("/run/systemd/timesync/synchronized", F_OK); + if (r < 0 && errno != ENOENT) + log_debug_errno(errno, "Failed to determine whether /run/systemd/timesync/synchronized exists, ignoring: %m"); + m->synchronized = r >= 0; + r = sd_resolve_default(&m->resolve); if (r < 0) return r; diff --git a/src/timesync/timesyncd-manager.h b/src/timesync/timesyncd-manager.h index 6244e07592..74917aa0ee 100644 --- a/src/timesync/timesyncd-manager.h +++ b/src/timesync/timesyncd-manager.h @@ -104,6 +104,9 @@ struct Manager { struct timespec origin_time, dest_time; bool spike; + /* Indicates whether we ever managed to set the local clock from NTP */ + bool synchronized; + /* save time event */ sd_event_source *event_save_time; usec_t save_time_interval_usec; From 922a65082934c6169cd02f489edc2484b2c0a807 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Mar 2022 16:58:26 +0100 Subject: [PATCH 16/16] timesyncd: improve log message whe getting a reply from server The message is misleading: it's not about synchronization but about successful communicaiton. And it's not about "initial", but only about first contact since we siwtched to this server. Let's hence reword the message a bit. --- src/timesync/timesyncd-manager.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index ba7e52f6b3..41c18b5e86 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -611,9 +611,8 @@ static int manager_receive_response(sd_event_source *source, int fd, uint32_t re (void) server_address_pretty(m->current_server_address, &pretty); - /* "Initial", as further successful syncs will not be logged. */ - log_info("Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); - (void) sd_notifyf(false, "STATUS=Initial synchronization to time server %s (%s).", strna(pretty), m->current_server_name->string); + log_info("Contacted time server %s (%s).", strna(pretty), m->current_server_name->string); + (void) sd_notifyf(false, "STATUS=Contacted time server %s (%s).", strna(pretty), m->current_server_name->string); } if (!spike && !m->synchronized) {