From: Ton V. <ton...@op...> - 2010-03-22 12:43:46
|
On 20 Mar 2010, at 01:33, Holger Weiß wrote: > * Albrecht Dreß <alb...@ar...> [2010-03-18 19:29]: >> <snip> >> t=localtime((time_t *)&test_time); >> test_time_year=t->tm_year; >> test_time_mon=t->tm_mon; >> test_time_mday=t->tm_mday; >> test_time_wday=t->tm_wday; >> >> /* calculate the start of the day (midnight, 00:00 hours) >> when the specified test time occurs */ >> t->tm_sec=0; >> t->tm_min=0; >> t->tm_hour=0; >> /* Removed for the moment. This fixes a bug where the >> timeperiod is incorrectly calculated */ >> /* See t-tap/test_timeperiods for a test failure */ >> /* t->tm_isdst=-1; */ >> midnight=(unsigned long)mktime(t); >> </snip> >> >> Regarding the tm_isdst field, this code "initialises" it implicitly >> by >> calling localtime() but then assumes for the calculation of midnight >> that the dst setting is still correct which may be plain wrong. >> >> Example: test_time is 1269734400 (Mar 28 01:00:00 2010 CET), midnight >> will correctly be calculated as Mar 28 00:00:00 2010. If test_time >> is >> 1269763200 (Mar 28 10:00:00 2010 CEST), midnight is calculated as >> Mar 27 >> 23:00:00 2010 which is obviously wrong. This miscalculation is >> fixed, >> again, by properly initialising tm_isdst to -1. >> >> Thus, either the "tests" missed the dst change cases, or the real >> use of >> midnight is not midnight but something else. > > The real use of the midnight value is not "12 o'clock at night," but > "the zero point of the local time of the day," which differs from "12 > o'clock at night" if tm_isdst changed since 12 o'clock at night. That > is, the offset of 10:00 AM from this zero point is 36,000 seconds, > regardless of whether tm_isdst changed since 12 o'clock at night. > >> In the latter case, the fix is of course to re-write the time span >> calculation to work properly (taking into account that days may >> also be >> 23 or 25 hours long, when dst changes) > > What makes you believe that the code in question does not work > properly? Thankfully, Holger has opened my eyes to my "revert the tm_isdst=-1" position. Holger said (on IRC) "there were two different cases in the code: (1) at most places in the code, tm_isdst was initialized just fine (by localtime(3)), but (2) at other places, it was indeed used uninitialized". I've already fixed on occurrence of (1) (in check_time_against_period()), and I think I've found the other occurrence now (in get_next_valid_time()). This is now fixed with updated test cases in test_timeperiods.c. There is one particular test that fails at the moment which I'm not sure how to fix - any ideas would be appreciated (it is marked as a TODO so full tests will not fail). I think all other occurrences of tm_isdst=-1 are of type (2), so I am not planning on reverting those anymore. Ton |