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].
