Re: [Nagios-devel] Fix for mktime() issue

[Guest]

--Apple-Mail-119-603238181
Content-Type: text/plain;
charset=ISO-8859-1;
format=flowed;
delsp=yes
Content-Transfer-Encoding: quoted-printable


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

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

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

t-tap/test_timeperiods.c says that, for the specific case of this bit =20=

of code in base/utils.c:

/* calculate the start of the day (midnight, 00:00 hours) =20
when the specified test time occurs */
t->tm_sec=3D0;
t->tm_min=3D0;
t->tm_hour=3D0;
/* Removed for the moment. This fixes a bug where the =20
timeperiod is incorrectly calculated */
/* See t-tap/test_timeperiods for a test failure */
/* t->tm_isdst=3D-1; */

that there are failures when running test_timeperiods when tm_isdst is =20=

set to -1. You can argue with me if the test is wrong (I don't think =20
so), but if the test is right, then setting to -1 leads to errors. Try =20=

it and see!

>> What I don't have, are tests for the other 18 cases where is_dst =20
>> has been initialised, and so I didn't revert those other changes. =20
>> But my opinion is now that this is too cautious and we should just =20=

>> revert back to Nagios pre-3.2.0 behaviour.
>
> If you don't believe me, run the attached little C test app which =20
> takes the current time, simulates your approach of the unitialised =20
> field, and converts back, which should of course give the initial =20
> value. For me (CET, no dst in effect) the code always fails =20
> (=3D=3Dproduces an off-by-one-hour error) if the tm_isdst is > 0. I =20=

> don't think you will find any box with time zone !=3D UTC where it =20
> doesn't...

That test is irrelevant because you are testing the spec, not testing =20=

Nagios behaviour. If you can create or update a test like =20
test_timeperiods.c which actually tests the functions within Nagios, =20
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 =20=

of the day based on the timeperiods in the configuration file
2. When calculating if a timeperiod matches, it takes the current =20
top of the day and the current offset seconds from the top and sees if =20=

it falls within the top and bottom of the offsets
3. The bug happens on the day of the DST (going backwards) because =20=

there are 25 hours in that day, so from 11pm onwards, this does not =20
fit in the "offset of that day" algorithm

test_timeperiod.c is testing the inputs and outputs of the =20
check_time_against_period function. Arguably, the logic should be =20
different, but if you want to refactor, then you will still need the =20
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 =20
found, but I am saying that a wholesale change of isdst=3D-1 is causing =20=

other problems. I've proven the single case, but I haven't written the =20=

tests for the other 16 cases when isdst is set.

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

M

...[email truncated]...


This post was automatically imported from historical nagios-devel mailing list archives
Original poster: [email protected]