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

> + int
> + ppsratecheck(lasttime, curpps, maxpps)
> + 	struct timeval *lasttime;
> + 	int *curpps;
> + 	int maxpps;	/* maximum pps allowed */

*curpps and maxpps should be unsigned.

> + {
> + 	s = splclock(); 
> + 	timersub(&mono_time, lasttime, &delta);

splclock() should only protect reference to mono_time, not whole the
function.

> + 	/*
> + 	 * 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.

> + 		*lasttime = mono_time;

mono_time should be cached at the beginning of the function.

> + 		*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)

> + 	splx(s);
> + 
> + 	return (rv);
> + }

Atsushi