Re: [chrony-dev] [PATCH v3 4/5] leapdb: support leap-seconds.list as second source
On Tue, Jan 30, 2024 at 2:23 AM Miroslav Lichvar wrote: > > On Thu, Dec 07, 2023 at 01:17:15PM +1100, [email protected] wrote: > > This patch adds support for getting leap second information from the > > leap-seconds.list file included with tzdata and adds a new configuration > > directive leapseclist to switch on the feature. > > Finally getting back to this patch set. Thanks for the review, I'll submit an updated series in a few days. Patrick -- To unsubscribe email [email protected] with "unsubscribe" in the subject. For help email [email protected] with "help" in the subject. Trouble? Email [email protected].
Re: [chrony-dev] [PATCH v3 4/5] leapdb: support leap-seconds.list as second source
On Thu, Dec 07, 2023 at 01:17:15PM +1100, [email protected] wrote: > This patch adds support for getting leap second information from the > leap-seconds.list file included with tzdata and adds a new configuration > directive leapseclist to switch on the feature. Finally getting back to this patch set. > diff --git a/doc/chrony.conf.adoc b/doc/chrony.conf.adoc > +[[leapseclist]]*leapseclist* _file_:: > +This directive specifies the path to a file containing a list of leap > seconds. Can you please add few words about the file format, maybe simply describe it as the NIST format? It contains not just the leap seconds, but also the TAI-UTC offset. > diff --git a/leapdb.c b/leapdb.c > +enum { > + SRC_NONE, > + SRC_TZ, > + SRC_LEAP_SEC_LIST, > +} leap_src; The enum naming seems inconsistent. Maybe "SRC_TIMEZONE" and "SRC_LIST" would look better? > + > +/* Offset between leap-seconds.list timestamp epoch and Unix epoch. > + leap-seconds.list epoch is 1 Jan 1900, 00:00:00 */ > +static const long long LEAP_SEC_LIST_OFFSET = 2208988800; This should be a macro. > @@ -93,6 +102,78 @@ get_tz_leap(time_t when, int *tai_offset) > > /* == */ > > +static NTP_Leap > +get_leap_sec_list_leap(time_t when, int *tai_offset) Or shorter get_list_leap()? > +{ > + FILE *f; > + char *line = NULL; > + size_t linelen = 0; > + NTP_Leap lsl_leap = LEAP_Normal; > + int prev_lsl_tai_offset = 10; > + long long lsl_updated = 0, lsl_expiry = 0; I think these should be int64_t and SCNd64 format in sscanf. > + if (!(f = fopen(CNF_GetLeapSecList(), "r"))) { Use UTI_OpenFile() here. It will print an error message. And maybe add a variable to save the return value of CNF_GetLeapSecList(), so it doesn't have to be called multiple times in the same function? > + while (getline(&line, &linelen, f) > 0) { Please use fgets() as used in all other file parsing. > +long long lsl_when; int64_t > +int lsl_tai_offset; > + > +if (*line == '#') { > + /* Update time line starts with #$ */ > + if (line[1] == '$' && sscanf(line + 2, "%lld", &lsl_updated) != 1) > +goto error; > + /* Expiration time line starts with #@ */ > + if (line[1] == '@' && sscanf(line + 2, "%lld", &lsl_expiry) != 1) > +goto error; > + /* Comment or a special comment we don't care about */ > + continue; > +} > + > +/* Leap entry */ > +if (sscanf(line, "%lld %d", &lsl_when, &lsl_tai_offset) != 2) > + goto error; > + > +if (when == lsl_when) { > + if (lsl_tai_offset > prev_lsl_tai_offset) > +lsl_leap = LEAP_InsertSecond; > + else if (lsl_tai_offset < prev_lsl_tai_offset) > +lsl_leap = LEAP_DeleteSecond; > + /* When is rounded to the end of the day, so offset hasn't changed > yet! */ > + *tai_offset = prev_lsl_tai_offset; > +} else if (when > lsl_when) > + *tai_offset = lsl_tai_offset; > + > +prev_lsl_tai_offset = lsl_tai_offset; > + } > + > + /* Make sure the file looks sensible */ > + if (!feof(f) || !lsl_updated || !lsl_expiry) > +goto error; > + > + if (when >= lsl_expiry) > +LOG(LOGS_WARN, "Leap second list %s needs update", CNF_GetLeapSecList()); > + > + goto out; > + > +error: > + LOG(LOGS_ERR, "Failed to parse leap seconds list %s", > CNF_GetLeapSecList()); > +out: > + if (f) > +fclose(f); > + return lsl_leap; > +} > + > +/* == */ > + > static int > check_leap_source(NTP_Leap (*src)(time_t when, int *tai_offset)) > { > @@ -111,14 +192,30 @@ check_leap_source(NTP_Leap (*src)(time_t when, int > *tai_offset)) > void > LDB_Initialise(void) > { > - leap_tzname = CNF_GetLeapSecTimezone(); > + const char *leap_tzname = CNF_GetLeapSecTimezone(); >if (leap_tzname && !check_leap_source(get_tz_leap)) { > LOG(LOGS_WARN, "Timezone %s failed leap second check, ignoring", > leap_tzname); > leap_tzname = NULL; >} > > - if (leap_tzname) > + const char *leap_sec_list = CNF_GetLeapSecList(); Declarations should be at the beginning of the function. > + if (leap_sec_list && !check_leap_source(get_leap_sec_list_leap)) { > +LOG(LOGS_WARN, "Leap second list %s failed check, ignoring", > leap_sec_list); > +leap_sec_list = NULL; > + } > + > + if (leap_sec_list && leap_tzname) { > +LOG(LOGS_WARN, "Multiple leap second sources, ignoring leapsectz"); > +leap_tzname = NULL; This warning doesn't seem necessary to me. Info message for the first working source is fine. It doesn't have to check both. Thanks, -- Miroslav Lichvar -- To unsubscribe email [email protected] with "unsubscribe" in the subject. For help email [email protected] with "help" in the subject. Trouble? Email [email protected].
[chrony-dev] [PATCH v3 4/5] leapdb: support leap-seconds.list as second source
From: Patrick Oppenlander
The existing implementation of getting leap second information from a
timezone in get_tz_leap() relies on non-portable C library behaviour.
Specifically, mktime is not required to return '60' in the tm_sec field
when a leap second is inserted leading to "Timezone right/UTC failed
leap second check, ignoring" errors on musl based systems.
This patch adds support for getting leap second information from the
leap-seconds.list file included with tzdata and adds a new configuration
directive leapseclist to switch on the feature.
---
conf.c | 14 ++
conf.h | 1 +
doc/chrony.conf.adoc | 19 +--
leapdb.c | 117 ---
4 files changed, 142 insertions(+), 9 deletions(-)
diff --git a/conf.c b/conf.c
index fa74459..06857a8 100644
--- a/conf.c
+++ b/conf.c
@@ -249,6 +249,9 @@ static REF_LeapMode leapsec_mode = REF_LeapModeSystem;
/* Name of a system timezone containing leap seconds occuring at midnight */
static char *leapsec_tz = NULL;
+/* File name of leap seconds list, usually
/usr/share/zoneinfo/leap-seconds.list */
+static char *leapsec_list = NULL;
+
/* Name of the user to which will be dropped root privileges. */
static char *user;
@@ -471,6 +474,7 @@ CNF_Finalise(void)
Free(hwclock_file);
Free(keys_file);
Free(leapsec_tz);
+ Free(leapsec_list);
Free(logdir);
Free(bind_ntp_iface);
Free(bind_acq_iface);
@@ -620,6 +624,8 @@ CNF_ParseLine(const char *filename, int number, char *line)
parse_leapsecmode(p);
} else if (!strcasecmp(command, "leapsectz")) {
parse_string(p, &leapsec_tz);
+ } else if (!strcasecmp(command, "leapseclist")) {
+parse_string(p, &leapsec_list);
} else if (!strcasecmp(command, "local")) {
parse_local(p);
} else if (!strcasecmp(command, "lock_all")) {
@@ -2386,6 +2392,14 @@ CNF_GetLeapSecTimezone(void)
/* == */
+char *
+CNF_GetLeapSecList(void)
+{
+ return leapsec_list;
+}
+
+/* == */
+
int
CNF_GetSchedPriority(void)
{
diff --git a/conf.h b/conf.h
index 58ebdeb..4c0a787 100644
--- a/conf.h
+++ b/conf.h
@@ -91,6 +91,7 @@ extern char *CNF_GetNtpSigndSocket(void);
extern char *CNF_GetPidFile(void);
extern REF_LeapMode CNF_GetLeapSecMode(void);
extern char *CNF_GetLeapSecTimezone(void);
+extern char *CNF_GetLeapSecList(void);
/* Value returned in ppm, as read from file */
extern double CNF_GetMaxUpdateSkew(void);
diff --git a/doc/chrony.conf.adoc b/doc/chrony.conf.adoc
index cb3f95c..efb659d 100644
--- a/doc/chrony.conf.adoc
+++ b/doc/chrony.conf.adoc
@@ -674,9 +674,10 @@ trusted and required source.
*tai*:::
This option indicates that the reference clock keeps time in TAI instead of UTC
and that *chronyd* should correct its offset by the current TAI-UTC offset. The
-<> directive must be used with this option and the
-database must be kept up to date in order for this correction to work as
-expected. This option does not make sense with PPS refclocks.
+<> or <> directive must be
+used with this option and the database must be kept up to date in order for
+this correction to work as expected. This option does not make sense with PPS
+refclocks.
*local*:::
This option specifies that the reference clock is an unsynchronised clock which
is more stable than the system clock (e.g. TCXO, OCXO, or atomic clock) and
@@ -1263,6 +1264,18 @@ $ TZ=right/UTC date -d 'Dec 31 2008 23:59:60'
Wed Dec 31 23:59:60 UTC 2008
+[[leapseclist]]*leapseclist* _file_::
+This directive specifies the path to a file containing a list of leap seconds.
+It is recommended to use the file _leap-seconds.list_ usually included with the
+system timezone database. The behaviour of this directive is otherwise
equivalent
+to <>.
++
+An example of this directive is:
++
+
+leapseclist /usr/share/zoneinfo/leap-seconds.list
+
+
[[makestep]]*makestep* _threshold_ _limit_::
Normally *chronyd* will cause the system to gradually correct any time offset,
by slowing down or speeding up the clock as required. In certain situations,
diff --git a/leapdb.c b/leapdb.c
index c8a42cb..bab1663 100644
--- a/leapdb.c
+++ b/leapdb.c
@@ -3,6 +3,7 @@
**
* Copyright (C) Miroslav Lichvar 2009-2018, 2020, 2022
+ * Copyright (C) Patrick Oppenlander 2023
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of version 2 of the GNU General Public License as
@@ -33,8 +34,16 @@
/* == */
-/* Name of a system timezone containing leap seconds occuring at midnight */
-static char *leap_tzname;
+/* Source of leap second data */
+enum {
+ SRC_NONE,
+ SRC_TZ,
+ SRC_LEAP_SEC_LIST,
+} leap_src;
+
+/* Offset between leap-seconds.list timestamp epoch and Unix epoch.
+ leap-seconds.list epoch is
