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

[Patrick Oppenlander <patrick.oppenlander@xxxxxxxxx>]

On Wed, Feb 7, 2024 at 9:35 PM Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:
>
> On Wed, Feb 07, 2024 at 04:29:43PM +1100, patrick.oppenlander@xxxxxxxxx wrote:
> > +  while (fgets(line, sizeof line, f) > 0) {
> > +    int64_t lsl_when;
> > +    int lsl_tai_offset;
> > +    char *p;
> > +
> > +    /* Ignore blank lines */
> > +    for (p = line; *p && isspace(*p); ++p);
> > +    if (!*p)
> > +      continue;
> > +
> > +    if (*line == '#') {
> > +      /* Update time line starts with #$ */
> > +      if (line[1] == '$' && sscanf(line + 2, "%"SCNd64, &lsl_updated) != 1)
> > +        goto error;
> > +      /* Expiration time line starts with #@ */
> > +      if (line[1] == '@' && sscanf(line + 2, "%"SCNd64, &lsl_expiry) != 1)
> > +        goto error;
> > +      /* Comment or a special comment we don't care about */
> > +      continue;
> > +    }
> > +
> > +    /* Leap entry */
> > +    if (sscanf(line, "%"SCNd64" %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;
>
> The error path taken here and in the loop logs an error message, but
> the leap and TAI offset values can already be accepted at that point.
> I think either they should be saved to temporary variables, or the
> loop should stop when a matching entry is found, so the error message
> is not logged when the result is used.

Thanks for pointing that out. I'll go with temporary variables. I
think it's better not to use the values if the file looks bogus.

> Everything else looks good to me. Thanks for your work on this.

Thanks for spending the time to review it,

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.