Subject: Re: ppsratecheck(9)
To: Atsushi Onoe <onoe@sm.sony.co.jp>
From: None <itojun@iijlab.net>
List: tech-kern
Date: 07/07/2000 14:54:39
	thanks for comments.

>> + int
>> + ppsratecheck(lasttime, curpps, maxpps)
>> + 	struct timeval *lasttime;
>> + 	int *curpps;
>> + 	int maxpps;	/* maximum pps allowed */
>*curpps and maxpps should be unsigned.

	maybe, maybe not.  anyone care about more than 2^31 events?
	if really so, long or u_long.

>> + {
>> + 	s = splclock(); 
>> + 	timersub(&mono_time, lasttime, &delta);
>splclock() should only protect reference to mono_time, not whole the
>function.
>> + 		*lasttime = mono_time;
>mono_time should be cached at the beginning of the function.

	i took this part from ratecheck(9).

>> + 	/*
>> + 	 * check for 0,0 is so that the message will be seen at least once.
>> + 	 * if more than one second have passed since the last update of
>> + 	 * lasttime, start over.
>> + 	 */
>> + 	if ((lasttime->tv_sec == 0 && lasttime->tv_usec == 0) ||
>> + 	    delta.tv_sec > 0) {
>delta might be negative if administrator reset the date by hand.
>delta.tv_sec != 0 looks more appropriate.

	i don't think so.
	(1) mono_time will never go back.  mono_time(9).
	(2) even if it goes back, it is inappropriate to reset *lasttime
	    when delta.tv_sec < 0.

>> + 		*curpps = 0;
>> + 		rv = 1;
>> + 	} else if (*curpps < maxpps)
>> + 		rv = 1;
>> + 	else
>> + 		rv = 0;
>> + 	/* be careful about wrap-around */
>> + 	if (*curpps + 1 > *curpps)
>> + 		*curpps = *curpps + 1;
>Since *curpps should be unsigned, this would be
>	if (*curpps + 1 != 0)

 	if (*curpps + 1 > *curpps) should do for both signed and unsigned.

itojun