From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id DA3373858D37 for ; Wed, 17 Aug 2022 21:50:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DA3373858D37 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-8-lVQzHsmiPXWCAe31_CNjvw-1; Wed, 17 Aug 2022 17:50:14 -0400 X-MC-Unique: lVQzHsmiPXWCAe31_CNjvw-1 Received: by mail-qk1-f197.google.com with SMTP id l15-20020a05620a28cf00b006b46997c070so12904956qkp.20 for ; Wed, 17 Aug 2022 14:50:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=uQNOqsHf91plPazZNtq95CT2Q+XTejxVDu0jkPLFiL4=; b=sxe5oso4qtnYWDjzIyd9SjrpC4Qjd2+XnQ1AICe61Abe08BP5+5xENKzyIjvlsGmE8 6BfrbYH3Ezny06IRbUknWojF/16ZKUqocaw92f0V0mf/njKsm2fq4DPpCTp2bWf5nBE9 GhdIZaxlDg8rvzabLa6z2LO2floV1Ntk4VR1NQ0Jm/FRuzd/s52/WSlGm5m/rVTHYM7I 5fYNJqMxyQwY5oQO8vJcoyGg2LnxOrKuJpyFEHymIm9ZuHDOB5oLd5xRIbIi50rMx2n9 LliSuZQgRLUJgyiJZCUc9KWk6KeY1KPIcvFFhuPutAsRPEkwWfk3Mja9joSUAz7dEwIf DzGw== X-Gm-Message-State: ACgBeo0BCNDH36GkIPuYjpWjQNxYaG/yHtXUV/06YHWTy+QxGGCOKB4P W+tPLNfRV/eB687DV10dW5BMBxhhkgQ1wpfwJXC1pXBCsXmTxXyKIAOVEnMg4y9nnOVi/wdbmZg 0zEUnsy8mCAKw3sCWRce1 X-Received: by 2002:a05:620a:319d:b0:6b8:d6e9:bb43 with SMTP id bi29-20020a05620a319d00b006b8d6e9bb43mr68758qkb.248.1660773014015; Wed, 17 Aug 2022 14:50:14 -0700 (PDT) X-Google-Smtp-Source: AA6agR6C98tyfEYbBD+Wz3vlif+aAu0PMGz8pJSHGCEKXANrfcLGYYtkj0u5iXJkJtbUSgBBZRSz6w== X-Received: by 2002:a05:620a:319d:b0:6b8:d6e9:bb43 with SMTP id bi29-20020a05620a319d00b006b8d6e9bb43mr68745qkb.248.1660773013679; Wed, 17 Aug 2022 14:50:13 -0700 (PDT) Received: from [192.168.0.241] (192-0-145-146.cpe.teksavvy.com. [192.0.145.146]) by smtp.gmail.com with ESMTPSA id j17-20020ac86651000000b003436103df40sm12808131qtp.8.2022.08.17.14.50.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Aug 2022 14:50:13 -0700 (PDT) Message-ID: <7cd72cc3-96d4-6c0f-f761-b2c0c70db85e@redhat.com> Date: Wed, 17 Aug 2022 17:50:12 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [swbz 29035] mktime vs non-DST To: DJ Delorie , libc-alpha@sourceware.org, Paul Eggert References: From: Carlos O'Donell Organization: Red Hat In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2022 21:50:17 -0000 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.