Re: [swbz 29035] mktime vs non-DST

[Carlos O'Donell <carlos@redhat.com>]

On 8/17/22 17:18, DJ Delorie via Libc-alpha wrote:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=29035
> 
> TL;DR - requesting a partial reversion of 86aece3 to become
> bug-compatible with older releases.
> 
> Long version:
> 
> In investigating this, I did a deep-dive on how tm_isdst works in
> mktime().  It seems to be less of a hint and more of an override, in
> that, if you set tm_isdst=1 you're going to get a result that seems an
> hour off if you're in the middle of a standard time period.  Same for
> tm_isdst=0.  Setting tm_isdst=-1 is the only way to let mktime use the
> dst-in-effect for the time period specified.  Note: I'm not
> considering the time duplication that happens at period boundaries
> (i.e. the "fall back" that causes an hour of clock time to repeat each
> fall).
> 
> So if you set tm_isdst=1 in a call to mktime(), it figures out the
> local DST offset and applies it regardless of timezone rules.
> 
> In the BZ case, however, the zoneinfo in effect does not have a DST
> defined (or, as we'll see later, hasn't had DST in a long time).  If
> there's no DST, and you set tm_isdst=1, what happens?
> 
> Well, prior to 2.29, mktime just overrode tm_isdst and returned a
> suitable time according to the current zoneinfo, as if you had passed
> tm_isdst=0 or -1 instead.

We should continue to do that until the end of time.

No matter what the standards say, at this point the behaviour of mktime() when
passed tm_isdst=0 or tm_isdst=1 is a contract with our users.

> As of 2.29, we have commit 86aece3bfbd44538ba4fdc947872c81d4c5e6e61
> by Paul which includes:
> 
>     (__mktime_internal): Set errno to EOVERFLOW if the spring-forward
>     gap code fails.
> 
>    /* We have a match.  Check whether tm.tm_isdst has the requested
>       value, if any.  */
>    if (isdst_differ (isdst, tm.tm_isdst))
>      {
>        . . .
> +      __set_errno (EOVERFLOW);
> +      return -1;
>      }
> 
> With this change, tm_isdst becomes a hard requirement, and if the
> current zone doesn't have a DST defined, you get a failure, where we
> used to succeed (but with a non-DST result).

We should do this.

> The relevent standards are pretty quiet on this topic; what little
> they say can be interpreted either way - tm_isdst is a requirement, or
> tm_isdst is a hint to be corrected by mktime() like other fields.

We should do and keep doing whatever the old code did IMO and document that.

> This breaks the logic down into three categories:
> 
> 1. You're in a transition period where clock time repeats, and you
>    need tm_isdst to decide which to return.
> 
> 2. You're not in a transition period, and you might as well set
>    tm_isdst=-1 unless you want an off-by-an-hour result.
> 
> 3. Your zone doesn't have dst and setting tm_isdst=1 is meaningless.

Consider the application programmer point of view.

They want to always take a specific action like (1), so they just set tm_isdst=1
to ensure we always pick one side of the transition.

You would never set -1.

You would always set, say 1, regardless of the zone.

You expect to always get an answer, never an error, and get a reasonable result.
 
> I can't see an obvious way to detect case 1 from 2, so this seems to
> be a useless set of categories.  A better breakdown would be:
> 
> 1. You set tm_isdst=-1 by default.  Most of the time, this works.

Right.

> 2. If the time is ambiguous due to a transition, case 1 returns EAGAIN
>    and you try again with tm_isdst=0 or 1.

Right.

> 3. If you set tm_isdst=0 or 1 outside of a transition, it returns
>    EINVAL if it's incorrect for that time.

Right.

I agree with you here, but let me introduce what I believe users want:

1. You set tm_isdst=-1 by default and it always works. The algorithm picks
   one of the times in the transition zone for you by default.

2. You set tm_isdst=0/1 and it always works, but the value selects one side
   of the transition if you're in one, otherwise it behaves like -1.

> But that would be a BIG world-breaking change.  One can dream :-)

Lets leave -1 aside for now.

Setting to 0 or 1 should just not fail IMO.

We should work to restore the existing old behaviour, and make sure that
between 2.29 and the future that we don't regress.
 
> Meanwhile, I would like us to consider reverting the commit mentioned
> above (not the whole commit, just the two lines I included).  This
> will have the effect of making the current code bug-compatible with
> older code, in that, setting tm_isdst=1 in a no-dst zone returns a
> non-dst (but otherwise valid) time, and updates tm_idst to 0.
> Returning EOVERFLOW in these new cases is not useful.
 
Agreed. Lets do that. And add tests please.

-- 
Cheers,
Carlos.