Fix for mktime() issue
Ton Voon
ton.voon at opsera.com
Wed Mar 17 11:12:10 CET 2010
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
More information about the Developers
mailing list