<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 6, 2022 at 5:49 PM Paul Eggert <<a href="mailto:eggert@cs.ucla.edu">eggert@cs.ucla.edu</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 6/6/22 15:56, enh wrote:<br>
<br>
> it's your mktime(), so you tell me :-)<br>
<br>
tzdb mktime doesn't make any attempt to preserve errno on success. Even <br>
on a platform like bionic with primitives following a <br>
preserve-errno-on-success philosophy, tzdb functions can succeed after <br>
calling other functions that may fail and set errno. For example, <br>
tzset_unlocked can recover from a failed zoneinit (which sets errno) by <br>
calling zoneinit again.<br>
<br>
> in bionic we use RAII to make functions "errno clean" all<br>
> the time to avoid this kind of problem because it's too hard to reason<br>
> about...)<br>
<br>
If we wanted to make tzdb localtime.c and strftime.c "errno clean" it'd <br>
require some careful work. It'd be doable but I expect it's not a <br>
one-hour job (though perhaps you could prove me wrong :-).<br></blockquote><div><br></div><div>i think worse than that, it's just too hard to reason about --- we'd bitrot anyway.</div><div><br></div><div>it's really not obvious that there's a good fix (beyond "wait for 32-bit to die") at all...</div><div><br></div><div>i don't think we can easily fix mktime64() --- i think we'd basically need to fix it differently, so we built tzcode twice (once with the public ABI 32-bit time_t and again with the 64-bit time_t), which at the very least would be quite a behavior change for any code using the 64-bit stuff. we'd also have to change the const-ness of a few arguments, which could be a breaking change. (i mean, it _is_ a breaking change; the question is really "how many apps would break?".)</div><div><br></div><div>i don't think we can really go the errno route. you've convinced me of that.</div><div><br></div><div>i don't see how we can save the localtime_r() comparisons --- our fundamental problem is that we don't have anything from mktime64() to compare against.</div><div><br></div><div>i think the safest choice for bionic is probably just "disable the new check and preserve the old behavior". given that you can only get into this mess by passing a bad `struct tm` to strftime(), and given that most use cases i'm aware of have literally just asked us to fill out that `struct tm` from a `time_t`, so -1 is almost certain to be "legit -1" rather than "error -1", i think i'm fine with that? especially since we haven't seen/heard any complaints about the existing behavior (we haven't yet shipped <a href="https://github.com/eggert/tz/commit/8b238ec54c09556eb2aa405c1741eedfd12c4a87">https://github.com/eggert/tz/commit/8b238ec54c09556eb2aa405c1741eedfd12c4a87</a>, despite it being from 2020 --- it's mingaleev's work trying to get back in sync that uncovered this).</div><div><br></div><div>it sounds from the checkin comment like there wasn't a motivating example for this behavior change, but let us know if you can think of an actual downside to not taking this change...</div></div></div>