Re: [chrony-dev] [PATCH 1/1] reference: support leap-seconds.list leap second source

[Patrick Oppenlander <patrick.oppenlander@xxxxxxxxx>]

Hi Miroslav,

On Wed, Nov 29, 2023 at 9:05 PM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
>
> On Wed, Nov 29, 2023 at 11:15:59AM +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 leapsecdb to switch on the feature.
>
> Good idea.
>
> I have some comments on the design first. Details can be sorted out
> later. Please don't expect this to be ready on first attempt.

In hindsight I should have marked this as an RFC patch.

> >  /* ================================================== */
> >
> > +/* Leap second database */
> > +struct leapdb {
> > +  long long updated;
> > +  long long expiry;
> > +  size_t len;
> > +  struct leap {
> > +    long long when;
> > +    int tai_offset;
> > +  } leap[];
> > +};
>
> No need to keep this information in memory. Just read the file for
> each request. It's only twice per day.
>
> > +  if (leap_db) {
> > +    /* Check that the leap database has good data for Jun 30 2012 and Dec 31 2012 */
> > +    if (get_db_leap(1341014400, &tai_offset) == LEAP_InsertSecond && tai_offset == 34 &&
> > +        get_db_leap(1356912000, &tai_offset) == LEAP_Normal && tai_offset == 35) {
> > +      LOG(LOGS_INFO, "Using leap second database %s", leap_db);
> > +    } else {
> > +      LOG(LOGS_WARN, "Leap second database %s failed check, ignoring", leap_db);
> > +      leap_db = NULL;
> > +    }
> > +  }
>
> Code like this should not be duplicated. I think there should be a
> wrapper around get_tz_leap() and get_db_leap() implementing the
> 12-hour caching logic and the callers shouldn't care which database is
> used.
>
> Also, this is not really relevant to "reference", so it might a good
> opportunity to move it out to separate file, e.g. leapdb.c. The API
> would be just three functions LDB_Initialise(), LDB_GetLeap(),
> LDB_Finalise(). The first commit would refactor the existing code into
> that. The second commit would add the leapfile parsing, documentation
> and some tests.

Thanks for the feedback, I'll prepare a v2.

Patrick

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

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