Fix for mktime() issue

[Ton Voon]

On 18 Mar 2010, at 18:29, Albrecht Dreß wrote:

> Am 18.03.10 03:09 schrieb(en) eponymous alias:
>> If you don't trust the mktime() implementation, then a few test  
>> cases run outside of Nagios itself should be able to prove that  
>> before the software is even built.
>
> That was the purpose of my test app.  It works as advertised by  
> POSIX/ISO 9899 on Linux 32 and 64 bit, and on Mac OS X 10.4.
>
> Basically Ton's approach as far as I understand it is like in  
> function check_time_against_period() of base/utils.c:
>
> <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.  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) instead of shifting midnight around.

It could be that the nagios code incorrectly calculating 1269763200 as  
23:00:00 is why the algorithm works (by time shifting everything back  
an hour). But I don't really care.

The fact is, the tests work with isdst unset.

 From comments in this thread, I don't think anyone has actually run  
the tests, so here are the steps:
   wget http://nagios.sourceforge.net/download/cvs/nagios-HEAD.tar.gz
   tar --gzip -xf nagios-HEAD.tar.gz
   cd nagios-HEAD
   ./configure --enable-libtap
   make nagios
   make cgis
   make test-tap
   cd t-tap && ./test_timeperiods

Go on, take 5 mins to do it. I'll wait.

Tests will pass with the current code (Nagios 3.2.1).

Edit utils.c, add the tm_isdst=-1 in at line 857, recompile utils.o  
and test_timeperiods and execute test_timeperiods and 4 out of 3006  
tests will fail (I neglected to add a time_t output in the comment.  
Committed now).

One of those failures is for TZ=Europe/London at 1256511661 = Sun Oct  
25 23:01:01 2009, which is the date clocks go back one hour in Europe.  
This is precisely the problem everyone saw last year.

If you come back and tell me that my tests are wrong (preferably with  
why and what they should be), I will humbly apologise, fix it and then  
crawl back under my rock. Alternatively, if they are right, I'd like  
to move forward with removing the other 16 cases of isdst=-1, with the  
policy of only making changes when someone writes test cases ala  
test_timeperiods to prove why the current code is wrong.

Let me spell this out: specs do not make an argument; test cases  
against the nagios functions do.

Ton



------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev