On Thu, Sep 29, 2005 at 10:47:02AM -0700, Juan Betty-MGIA2720 wrote:
> Wes,
> I modified and tested this patch a bit. This should be a better fix, what you
> think ?
I still have a bad feeling about this, but I can't put the finger on it.
Basically timeouts are used for two purposes:
a) To make something happen at wall clock time T
b) To make something happen N time units into the future
Now, your patch seems to ensure that if the wall time ever goes backwards then
the first queued entry is run, regardless of when it should be run, so if
it really should be run sometime next month it will be run now because the
wall time went backwards a few seconds, and that feels very wrong.
The conclusion probably is 'do not mess with the system clock' but sometimes
that is hard to avoid.
/MF
> --- snmp_alarm.c.old 2005-09-28 11:03:28.000000000 -0700
> +++ snmp_alarm.c 2005-09-29 09:24:14.000000000 -0700
> @@ -223,6 +223,8 @@
> return NULL;
> }
>
> +static timeval t_last_run_alarms={ 0, 0 };
> void
> run_alarms(void)
> {
> @@ -231,6 +233,17 @@
> unsigned int clientreg;
> struct timeval t_now;
>
> + int timeBackFlag = 0;
> + gettimeofday(&t_now, NULL);
> + if( (t_now.tv_sec < t_last_run_alarms.tv_sec) ||
> + ((t_now.tv_sec == t_last_run_alarms.tv_sec) && (t_now.tv_usec <
> t_last_run_alarms.tv_usec) )
> + ) { // we have a clock going back reset the timers
> + timeBackFlag =1;
> + }
> + t_last_run_alarms.tv_sec = t_now.tv_sec;
> + t_last_run_alarms.tv_usec = t_now.tv_usec;
> +
> /*
> * Loop through everything we have repeatedly looking for the next thing
> to
> * call until all events are finally in the future again.
> @@ -243,13 +256,14 @@
>
> gettimeofday(&t_now, NULL);
>
> - if ((a->t_next.tv_sec < t_now.tv_sec) ||
> + if ( timeBackFlag || (a->t_next.tv_sec < t_now.tv_sec) ||
> ((a->t_next.tv_sec == t_now.tv_sec) &&
> (a->t_next.tv_usec < t_now.tv_usec))) {
> clientreg = a->clientreg;
> DEBUGMSGTL(("snmp_alarm", "run alarm %d\n", clientreg));
> (*(a->thecallback)) (clientreg, a->clientarg);
> DEBUGMSGTL(("snmp_alarm", "alarm %d completed\n", clientreg));
> + timeBackFlag = 0;
>
>
>
> -----Original Message-----
> From: Juan Betty-MGIA2720
> Sent: Wednesday, September 28, 2005 2:13 PM
> To: 'Wes Hardaker'
> Cc: [email protected]
> Subject: RE: Patch for delaying of tasks attached to snmp_alarm_register
>
> I agreed with you. I thought about it when doing the fix, that's why the 1000
> was picked as a trick since the struct only addresses the sec and usec. (I
> think the factor of 100 could also work, but anyway.)
>
> It just allows the alarms (or tasks attach to the alarm) to run at least
> once. I think, according to the logic, the tasks time will re-sync again.
>
> -----Original Message-----
> From: Wes Hardaker [mailto:[EMAIL PROTECTED]
> Sent: Wednesday, September 28, 2005 1:52 PM
> To: Juan Betty-MGIA2720
> Cc: [email protected]
> Subject: Re: Patch for delaying of tasks attached to snmp_alarm_register
>
> >>>>> On Wed, 28 Sep 2005 12:48:34 -0700, Juan Betty-MGIA2720 <[EMAIL
> >>>>> PROTECTED]> said:
>
> Juan> I found out when setting the system clock backwards (not
> Juan> forwards), delaying of tasks that are registered with
> Juan> snmp_alarm_register. The tasks which are scheduled to run at
> Juan> certain time seem to be held back until the new time catches up
> Juan> with old time again. Didn't see anyone post a patch for this
> Juan> problem, so shared here is a quick patch implemented. There is
> Juan> probably a better solution, but this solved my problem.
>
> Looks like a good patch, but there are a couple of minor issues with
> it:
>
> 1) tv_usec is actualyl 1/1000000th of a tv_sec not 1/1000
> 2) doing math the way you are would require 1000000 * tv_sec, which
> will wrap a 32 bit integer. You can't do it that way unfortunately
> and have to treat the tests individually (hence the reason all the
> other code does a bunch of nasty if checks to verify one stamp is
> less than the other).
> 3) I didn't check the logic carefully so this may be wrong, but I
> think you're running every alarm if the time goes backward at all?
>
> --
> Wes Hardaker
> Sparta, Inc.
>
>
> -------------------------------------------------------
> This SF.Net email is sponsored by:
> Power Architecture Resource Center: Free content, downloads, discussions, and
> more. http://solutions.newsforge.com/ibmarch.tmpl
> _______________________________________________
> Net-snmp-coders mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
>
>
> -------------------------------------------------------
> This SF.Net email is sponsored by:
> Power Architecture Resource Center: Free content, downloads, discussions,
> and more. http://solutions.newsforge.com/ibmarch.tmpl
> _______________________________________________
> Net-snmp-coders mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
