Re: [chrony-dev] [PATCH v3 4/5] leapdb: support leap-seconds.list as second source

[Miroslav Lichvar <mlichvar@xxxxxxxxxx>]

On Thu, Dec 07, 2023 at 01:17:15PM +1100, patrick.oppenlander@xxxxxxxxx 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 chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.