Fix for mktime() issue

[Ton Voon]

On 16 Mar 2010, at 18:51, Albrecht Dreß wrote:

> Am 16.03.10 10:30 schrieb(en) Ton Voon:
>>> What should be the purpose of a test case if the code strictly  
>>> follows POSIX?
>> Because the test cases proves real-world usage, rather than some  
>> spec?
>
> I don't think that POSIX and ISO 9899 are just "some spec".  If my  
> proper usage of a well-defined library function produces weird  
> results, I wouldn't assume that I am right and all those IEEE and  
> ISO people (and glibc coders) are wrong.  The usual approach IMHO  
> would be looking at *my* code, which might be /slightly/ less  
> perfect at second glance...

I'm not disputing that the spec is what we should work to, but the  
proper test has to be how Nagios handles the scenario with known  
inputs (configuration + time) and outputs (whether the timeperiod  
matches or not).

t-tap/test_timeperiods.c says that, for the specific case of this bit  
of code in base/utils.c:

         /* 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; */

that there are failures when running test_timeperiods when tm_isdst is  
set to -1. You can argue with me if the test is wrong (I don't think  
so), but if the test is right, then setting to -1 leads to errors. Try  
it and see!

>> What I don't have, are tests for the other 18 cases where is_dst  
>> has been initialised, and so I didn't revert those other changes.  
>> But my opinion is now that this is too cautious and we should just  
>> revert back to Nagios pre-3.2.0 behaviour.
>
> If you don't believe me, run the attached little C test app which  
> takes the current time, simulates your approach of the unitialised  
> field, and converts back, which should of course give the initial  
> value.  For me (CET, no dst in effect) the code always fails  
> (==produces an off-by-one-hour error) if the tm_isdst is > 0.  I  
> don't think you will find any box with time zone != UTC where it  
> doesn't...

That test is irrelevant because you are testing the spec, not testing  
Nagios behaviour. If you can create or update a test like  
test_timeperiods.c which actually tests the functions within Nagios,  
that would be relevant.

Here's what I found from looking at the DST bug last year:

   1. Nagios, on startup, works out the offset in seconds from the top  
of the day based on the timeperiods in the configuration file
   2. When calculating if a timeperiod matches, it takes the current  
top of the day and the current offset seconds from the top and sees if  
it falls within the top and bottom of the offsets
   3. The bug happens on the day of the DST (going backwards) because  
there are 25 hours in that day, so from 11pm onwards, this does not  
fit in the "offset of that day" algorithm

test_timeperiod.c is testing the inputs and outputs of the  
check_time_against_period function. Arguably, the logic should be  
different, but if you want to refactor, then you will still need the  
tests to prove that you have handled all situations.

(In case you can't tell, I'm a testing fanatic!)

Now, I'm not disputing that there is some other bug that you have  
found, but I am saying that a wholesale change of isdst=-1 is causing  
other problems. I've proven the single case, but I haven't written the  
tests for the other 16 cases when isdst is set.

Hence my position: I didn't want to change the others without tests,  
but I think that is now too conservative and I suggest we change all  
of them back until the proof goes the other way - you prove why it  
SHOULD be isdst=-1 with appropriate test updates to test_timeperiod.c

My guess: you are correct that isdst needs to be set when doing  
calculations based on actual date ranges (such as "2007-01-01 -  
2008-02-01	09:00-17:00", rather than "mon 00:00-24:00" type format).  
But again, I want tests updated rather than arguments about how the  
spec works.

I think I'm right in saying that some other implementations of Nagios  
have reverted the entire patch for isdst=-1. Anyone care to chip in  
here?

Information about the test suite: http://wiki.nagios.org/index.php/Nagios_Core_Developer_Guidelines#Testing

Ton




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.monitoring-lists.org/archive/developers/attachments/20100317/a82e50ee/attachment.html>
-------------- next part --------------
------------------------------------------------------------------------------
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
-------------- next part --------------
_______________________________________________
Nagios-devel mailing list
Nagios-devel at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nagios-devel