Various timer improvements

Now that clock_t is an unsigned value, we can also allow the system
uptime to wrap.  Essentially, instead of using (a <= b) to see if time
a occurs no later than time b, we use (b - a <= CLOCK_MAX / 2).  The
latter value does not exist, so instead we add TMRDIFF_MAX for that
purpose.

We must therefore also avoid using values like 0 and LONG_MAX as
special values for absolute times.  This patch extends the libtimers
interface so that it no longer uses 0 to indicate "no timeout".
Similarly, TMR_NEVER is now used as special value only when
otherwise a relative time difference would be used.  A minix_timer
structure is now considered in use when it has a watchdog function set,
rather than when the absolute expiry time is not TMR_NEVER.  A few new
macros in <minix/timers.h> help with timer comparison and obtaining
properties from a minix_timer structure.

This patch also eliminates the union of timer arguments, instead using
the only union element that is only used (the integer).  This prevents
potential problems with e.g. live update.  The watchdog function
prototype is changed to pass in the argument value rather than a
pointer to the timer structure, since obtaining the argument value was
the only current use of the timer structure anyway.  The result is a
somewhat friendlier timers API.

The VFS select code required a few more invasive changes to restrict
the timer value to the new maximum, effectively matching the timer
code in PM.  As a side effect, select(2) has been changed to reject
invalid timeout values.  That required a change to the test set, which
relied on the previous, erroneous behavior.

Finally, while we're rewriting significant chunks of the timer code
anyway, also covert it to KNF and add a few more explanatory comments.

Change-Id: Id43165c3fbb140b32b90be2cca7f68dd646ea72e

minix/drivers/hid/pckbd/pckbd.c

@@ -44,7 +44,7 @@ static struct inputdriver pckbd_tab = {
  * The watchdog timer function, implementing all but the actual reset.
  */
 static void
-kbd_watchdog(minix_timer_t *UNUSED(tmrp))
+kbd_watchdog(int arg __unused)
 {
 	kbd_watchdog_set = 0;
 	if (!kbdout.avail)

minix/drivers/storage/ahci/ahci.c

@@ -232,7 +232,7 @@ static void port_set_cmd(struct port_state *ps, int cmd, cmd_fis_t *fis,
 	u8_t packet[ATAPI_PACKET_SIZE], prd_t *prdt, int nr_prds, int write);
 static void port_issue(struct port_state *ps, int cmd, clock_t timeout);
 static int port_exec(struct port_state *ps, int cmd, clock_t timeout);
-static void port_timeout(minix_timer_t *tp);
+static void port_timeout(int arg);
 static void port_disconnect(struct port_state *ps);
 
 static char *ahci_portname(struct port_state *ps);
@@ -1712,7 +1712,7 @@ static void port_intr(struct port_state *ps)
 /*===========================================================================*
  *				port_timeout				     *
  *===========================================================================*/
-static void port_timeout(minix_timer_t *tp)
+static void port_timeout(int arg)
 {
 	/* A timeout has occurred on this port. Figure out what the timeout is
 	 * for, and take appropriate action.
@@ -1720,8 +1720,8 @@ static void port_timeout(minix_timer_t *tp)
 	struct port_state *ps;
 	int port, cmd;
 
-	port = GET_PORT(tmr_arg(tp)->ta_int);
+	port = GET_PORT(arg);
-	cmd = GET_TAG(tmr_arg(tp)->ta_int);
+	cmd = GET_TAG(arg);
 
 	assert(port >= 0 && port < hba_state.nr_ports);
 

minix/drivers/storage/floppy/floppy.c

@@ -239,8 +239,8 @@ static u8_t f_results[MAX_RESULTS];/* the controller can give lots of output */
 static minix_timer_t f_tmr_timeout;		/* timer for various timeouts */
 static u32_t system_hz;		/* system clock frequency */
 static void f_expire_tmrs(clock_t stamp);
-static void stop_motor(minix_timer_t *tp);
+static void stop_motor(int arg);
-static void f_timeout(minix_timer_t *tp);
+static void f_timeout(int arg);
 
 static struct device *f_prepare(devminor_t device);
 static struct device *f_part(devminor_t minor);
@@ -784,13 +784,13 @@ static void start_motor(void)
 /*===========================================================================*
  *				stop_motor				     *
  *===========================================================================*/
-static void stop_motor(minix_timer_t *tp)
+static void stop_motor(int arg)
 {
 /* This routine is called from an alarm timer after several seconds have
  * elapsed with no floppy disk activity.  It turns the drive motor off.
  */
   int s;
-  motor_status &= ~(1 << tmr_arg(tp)->ta_int);
+  motor_status &= ~(1 << arg);
   if ((s=sys_outb(DOR, (motor_status << MOTOR_SHIFT) | ENABLE_INT)) != OK)
 	panic("Sys_outb in stop_motor() failed: %d", s);
 }
@@ -1198,7 +1198,7 @@ static int f_intr_wait(void)
 /*===========================================================================*
  *				f_timeout				     *
  *===========================================================================*/
-static void f_timeout(minix_timer_t *UNUSED(tp))
+static void f_timeout(int arg __unused)
 {
 /* This routine is called when a timer expires.  Usually to tell that a
  * motor has spun up, but also to forge an interrupt when it takes too long

minix/drivers/tty/pty/tty.c

@@ -23,7 +23,7 @@
 /* A device exists if at least its 'devread' function is defined. */
 #define tty_active(tp)	((tp)->tty_devread != NULL)
 
-static void tty_timed_out(minix_timer_t *tp);
+static void tty_timed_out(int arg);
 static void settimer(tty_t *tty_ptr, int enable);
 static void in_transfer(tty_t *tp);
 static int tty_echo(tty_t *tp, int ch);
@@ -1290,11 +1290,11 @@ static int tty_init(int UNUSED(type), sef_init_info_t *UNUSED(info))
 /*===========================================================================*
  *				tty_timed_out				     *
  *===========================================================================*/
-static void tty_timed_out(minix_timer_t *tp)
+static void tty_timed_out(int arg)
 {
 /* This timer has expired. Set the events flag, to force processing. */
   tty_t *tty_ptr;
-  tty_ptr = &tty_table[tmr_arg(tp)->ta_int];
+  tty_ptr = &tty_table[arg];
   tty_ptr->tty_min = 0;			/* force read to succeed */
   tty_ptr->tty_events = 1;		
 }

minix/drivers/tty/tty/arch/i386/console.c

@@ -128,7 +128,7 @@ static void flush(console_t *cons);
 static void parse_escape(console_t *cons, int c);
 static void scroll_screen(console_t *cons, int dir);
 static void set_6845(int reg, unsigned val);
-static void stop_beep(minix_timer_t *tmrp);
+static void stop_beep(int arg);
 static void cons_org0(void);
 static void disable_console(void);
 static void reenable_console(void);
@@ -895,7 +895,7 @@ clock_t dur;
 /*===========================================================================*
  *				stop_beep				     *
  *===========================================================================*/
-static void stop_beep(minix_timer_t *UNUSED(tmrp))
+static void stop_beep(int arg __unused)
 {
 /* Turn off the beeper by turning off bits 0 and 1 in PORT_B. */
   u32_t port_b_val;

minix/drivers/tty/tty/tty.c

@@ -59,7 +59,7 @@ unsigned long rs_irq_set = 0;
 
 struct kmessages kmess;
 
-static void tty_timed_out(minix_timer_t *tp);
+static void tty_timed_out(int arg);
 static void settimer(tty_t *tty_ptr, int enable);
 static void in_transfer(tty_t *tp);
 static int tty_echo(tty_t *tp, int ch);
@@ -1573,11 +1573,11 @@ static void tty_init()
 /*===========================================================================*
  *				tty_timed_out				     *
  *===========================================================================*/
-static void tty_timed_out(minix_timer_t *tp)
+static void tty_timed_out(int arg)
 {
 /* This timer has expired. Set the events flag, to force processing. */
   tty_t *tty_ptr;
-  tty_ptr = &tty_table[tmr_arg(tp)->ta_int];
+  tty_ptr = &tty_table[arg];
   tty_ptr->tty_min = 0;			/* force read to succeed */
   tty_ptr->tty_events = 1;		
 }

minix/include/minix/timers.h

@@ -17,50 +17,61 @@
 #include <limits.h>
 
 #include <sys/types.h>
+#include <minix/const.h>
 #include <minix/u64.h>
 #include <minix/minlib.h>
 #include <minix/endpoint.h>
 
-struct minix_timer;
+typedef void (*tmr_func_t)(int arg);
-typedef void (*tmr_func_t)(struct minix_timer *tp);
-typedef union { int ta_int; long ta_long; void *ta_ptr; } ixfer_tmr_arg_t;
 
 /* A minix_timer_t variable must be declare for each distinct timer to be used.
  * The timers watchdog function and expiration time are automatically set
- * by the library function tmrs_settimer, but its argument is not.
+ * by the library function tmrs_settimer, but its argument is not.  In general,
+ * the timer is in use when it has a callback function.
  */
 typedef struct minix_timer
 {
   struct minix_timer	*tmr_next;	/* next in a timer chain */
-  clock_t 	tmr_exp_time;	/* expiration time */
+  clock_t 		tmr_exp_time;	/* expiration time (absolute) */
   tmr_func_t		tmr_func;	/* function to call when expired */
-  ixfer_tmr_arg_t tmr_arg;	/* random argument */
+  int			tmr_arg;	/* integer argument */
 } minix_timer_t;
 
-/* Used when the timer is not active. */
+/*
-#define TMR_NEVER    ((clock_t) -1 < 0) ? ((clock_t) LONG_MAX) : ((clock_t) -1)
+ * Clock times may wrap.  Thus, we must only ever compare relative times, which
-#undef TMR_NEVER
+ * means they must be no more than half the total maximum time value apart.
-#define TMR_NEVER	((clock_t) LONG_MAX)
+ * The clock_t type is unsigned (int or long), thus we take half that.
+ */
+#define TMRDIFF_MAX		(INT_MAX)
+
+/* This value must be used only instead of a timer difference value. */
+#define TMR_NEVER		((clock_t)TMRDIFF_MAX + 1)
 
 /* These definitions can be used to set or get data from a timer variable. */
-#define tmr_arg(tp) (&(tp)->tmr_arg)
+#define tmr_exp_time(tp)	((tp)->tmr_exp_time)
-#define tmr_exp_time(tp) (&(tp)->tmr_exp_time)
+#define tmr_is_set(tp)		((tp)->tmr_func != NULL)
+/*
+ * tmr_is_first() returns TRUE iff the first given absolute time is sooner than
+ * or equal to the second given time.
+ */
+#define tmr_is_first(a,b)	((clock_t)(b) - (clock_t)(a) <= TMRDIFF_MAX)
+#define tmr_has_expired(tp,now)	tmr_is_first((tp)->tmr_exp_time, (now))
 
 /* Timers should be initialized once before they are being used. Be careful
  * not to reinitialize a timer that is in a list of timers, or the chain
  * will be broken.
  */
-#define tmr_inittimer(tp) (void)((tp)->tmr_exp_time = TMR_NEVER, \
+#define tmr_inittimer(tp) (void)((tp)->tmr_func = NULL, (tp)->tmr_next = NULL)
-	(tp)->tmr_next = NULL)
 
 /* The following generic timer management functions are available. They
  * can be used to operate on the lists of timers. Adding a timer to a list
  * automatically takes care of removing it.
  */
-clock_t tmrs_clrtimer(minix_timer_t **tmrs, minix_timer_t *tp, clock_t *new_head);
+int tmrs_settimer(minix_timer_t **tmrs, minix_timer_t *tp, clock_t exp_time,
-void tmrs_exptimers(minix_timer_t **tmrs, clock_t now, clock_t *new_head);
+	tmr_func_t watchdog, int arg, clock_t *old_head, clock_t *new_head);
-clock_t tmrs_settimer(minix_timer_t **tmrs, minix_timer_t *tp, clock_t exp_time,
+int tmrs_clrtimer(minix_timer_t **tmrs, minix_timer_t *tp, clock_t *old_head,
-	tmr_func_t watchdog, clock_t *new_head);
+	clock_t *new_head);
+int tmrs_exptimers(minix_timer_t **tmrs, clock_t now, clock_t *new_head);
 
 #define PRINT_STATS(cum_spenttime, cum_instances) {		\
 		if(ex64hi(cum_spenttime)) { util_stacktrace(); printf(" ( ??? %lu %lu)\n",	\
@@ -110,7 +121,7 @@ clock_t tmrs_settimer(minix_timer_t **tmrs, minix_timer_t *tp, clock_t exp_time,
  */
 
 void init_timer(minix_timer_t *tp);
-void set_timer(minix_timer_t *tp, int ticks, tmr_func_t watchdog, int arg);
+void set_timer(minix_timer_t *tp, clock_t ticks, tmr_func_t watchdog, int arg);
 void cancel_timer(minix_timer_t *tp);
 void expire_timers(clock_t now);
 

minix/kernel/arch/earm/pre_init.c

@@ -418,7 +418,7 @@ kinfo_t *pre_init(int argc, char **argv)
  * longer used and the "real" implementations are visible
  */
 void send_diag_sig(void) { }
-void minix_shutdown(minix_timer_t *t) { arch_shutdown(0); }
+void minix_shutdown(int how) { arch_shutdown(how); }
 void busy_delay_ms(int x) { }
 int raise(int n) { panic("raise(%d)\n", n); }
 int kern_phys_map_ptr( phys_bytes base_address, vir_bytes io_size, int vm_flags,

minix/kernel/arch/i386/arch_system.c

@@ -371,7 +371,7 @@ static void ser_debug(const int c)
 	switch(c)
 	{
 	case 'Q':
-		minix_shutdown(NULL);
+		minix_shutdown(0);
 		NOT_REACHABLE;
 #ifdef CONFIG_SMP
 	case 'B':

minix/kernel/arch/i386/pre_init.c

@@ -246,6 +246,6 @@ kinfo_t *pre_init(u32_t magic, u32_t ebx)
 }
 
 void send_diag_sig(void) { }
-void minix_shutdown(minix_timer_t *t) { arch_shutdown(0); }
+void minix_shutdown(int how) { arch_shutdown(how); }
 void busy_delay_ms(int x) { }
 int raise(int sig) { panic("raise(%d)\n", sig); }

minix/kernel/clock.c

@@ -36,7 +36,6 @@ static void load_update(void);
  * When a timer expires its watchdog function is run by the CLOCK task. 
  */
 static minix_timer_t *clock_timers;	/* queue of CLOCK timers */
-static clock_t next_timeout;	/* monotonic time that next timer expires */
 
 /* Number of ticks to adjust realtime by. A negative value implies slowing
  * down realtime, a positive value implies speeding it up.
@@ -154,12 +153,14 @@ int timer_int_handler(void)
 	load_update();
 
 	if (cpu_is_bsp(cpuid)) {
-		/* if a timer expired, notify the clock task */
+		/*
-		if ((next_timeout <= kclockinfo.uptime)) {
+		 * If a timer expired, notify the clock task.  Keep in mind
+		 * that clock tick values may overflow, so we must only look at
+		 * relative differences, and only if there are timers at all.
+		 */
+		if (clock_timers != NULL &&
+		    tmr_has_expired(clock_timers, kclockinfo.uptime))
 			tmrs_exptimers(&clock_timers, kclockinfo.uptime, NULL);
-			next_timeout = (clock_timers == NULL) ?
-				TMR_NEVER : clock_timers->tmr_exp_time;
-		}
 
 #ifdef DEBUG_SERIAL
 		if (kinfo.do_serial_debug)
@@ -230,14 +231,14 @@ time_t get_boottime(void)
 void set_kernel_timer(
   minix_timer_t *tp,			/* pointer to timer structure */
   clock_t exp_time,			/* expiration monotonic time */
-  tmr_func_t watchdog			/* watchdog to be called */
+  tmr_func_t watchdog,			/* watchdog to be called */
+  int arg				/* argument for watchdog function */
 )
 {
 /* Insert the new timer in the active timers list. Always update the 
  * next timeout time by setting it to the front of the active list.
  */
-  tmrs_settimer(&clock_timers, tp, exp_time, watchdog, NULL);
+  (void)tmrs_settimer(&clock_timers, tp, exp_time, watchdog, arg, NULL, NULL);
-  next_timeout = clock_timers->tmr_exp_time;
 }
 
 /*===========================================================================*
@@ -251,9 +252,8 @@ void reset_kernel_timer(
  * active and expired lists. Always update the next timeout time by setting
  * it to the front of the active list.
  */
-  tmrs_clrtimer(&clock_timers, tp, NULL);
+  if (tmr_is_set(tp))
-  next_timeout = (clock_timers == NULL) ? 
+	(void)tmrs_clrtimer(&clock_timers, tp, NULL, NULL);
-	TMR_NEVER : clock_timers->tmr_exp_time;
 }
 
 /*===========================================================================*

minix/kernel/main.c

@@ -361,19 +361,18 @@ void prepare_shutdown(const int how)
    * argument passes the shutdown status. 
    */
   printf("MINIX will now be shut down ...\n");
-  tmr_arg(&shutdown_timer)->ta_int = how;
+  set_kernel_timer(&shutdown_timer, get_monotonic() + system_hz,
-  set_kernel_timer(&shutdown_timer, get_monotonic() + system_hz, minix_shutdown);
+      minix_shutdown, how);
 }
 
 /*===========================================================================*
  *				shutdown 				     *
  *===========================================================================*/
-void minix_shutdown(minix_timer_t *tp)
+void minix_shutdown(int how)
 {
 /* This function is called from prepare_shutdown or stop_sequence to bring 
  * down MINIX.
  */
-  int how;
 
 #ifdef CONFIG_SMP
   /* 
@@ -389,8 +388,6 @@ void minix_shutdown(minix_timer_t *tp)
   hw_intr_disable_all();
   stop_local_timer();
 
-  how = tp ? tmr_arg(tp)->ta_int : 0;
-
   /* Show shutdown message */
   direct_cls();
   if((how & RB_POWERDOWN) == RB_POWERDOWN)

minix/kernel/proto.h

@@ -23,7 +23,7 @@ void set_adjtime_delta(int32_t);
 clock_t get_monotonic(void);
 void set_boottime(time_t);
 time_t get_boottime(void);
-void set_kernel_timer(minix_timer_t *tp, clock_t t, tmr_func_t f);
+void set_kernel_timer(minix_timer_t *tp, clock_t t, tmr_func_t f, int arg);
 void reset_kernel_timer(minix_timer_t *tp);
 void ser_dump_proc(void);
 
@@ -51,7 +51,7 @@ void fpu_sigcontext(struct proc *, struct sigframe_sigcontext *fr, struct
 #endif
 void kmain(kinfo_t *cbi);
 void prepare_shutdown(int how);
-__dead void minix_shutdown(minix_timer_t *tp);
+__dead void minix_shutdown(int how);
 void bsp_finish_booting(void);
 
 /* proc.c */

minix/kernel/system/do_setalarm.c

@@ -14,7 +14,7 @@
 
 #if USE_SETALARM
 
-static void cause_alarm(minix_timer_t *tp);
+static void cause_alarm(int proc_nr_e);
 
 /*===========================================================================*
  *				do_setalarm				     *
@@ -28,21 +28,19 @@ int do_setalarm(struct proc * caller, message * m_ptr)
   clock_t uptime;		/* placeholder for current uptime */
 
   /* Extract shared parameters from the request message. */
-  exp_time = m_ptr->m_lsys_krn_sys_setalarm.exp_time;		/* alarm's expiration time */
+  exp_time = m_ptr->m_lsys_krn_sys_setalarm.exp_time;
-  use_abs_time = m_ptr->m_lsys_krn_sys_setalarm.abs_time;	/* flag for absolute time */
+  use_abs_time = m_ptr->m_lsys_krn_sys_setalarm.abs_time;
   if (! (priv(caller)->s_flags & SYS_PROC)) return(EPERM);
 
   /* Get the timer structure and set the parameters for this alarm. */
   tp = &(priv(caller)->s_alarm_timer);
-  tmr_arg(tp)->ta_int = caller->p_endpoint;
-  tp->tmr_func = cause_alarm; 
 
   /* Return the ticks left on the previous alarm. */
   uptime = get_monotonic(); 
-  if (tp->tmr_exp_time == TMR_NEVER) {
+  if (!tmr_is_set(tp)) {
 	m_ptr->m_lsys_krn_sys_setalarm.time_left = TMR_NEVER;
-  } else if (uptime < tp->tmr_exp_time) {
+  } else if (tmr_is_first(uptime, tp->tmr_exp_time)) {
-      m_ptr->m_lsys_krn_sys_setalarm.time_left = (tp->tmr_exp_time - uptime);
+	m_ptr->m_lsys_krn_sys_setalarm.time_left = tp->tmr_exp_time - uptime;
   } else {
 	m_ptr->m_lsys_krn_sys_setalarm.time_left = 0;
   }
@@ -50,12 +48,17 @@ int do_setalarm(struct proc * caller, message * m_ptr)
   /* For the caller's convenience, also return the current time. */
   m_ptr->m_lsys_krn_sys_setalarm.uptime = uptime;
 
-  /* Finally, (re)set the timer depending on the expiration time. */
+  /*
-  if (exp_time == 0) {
+   * Finally, (re)set the timer depending on the expiration time.  Note that
+   * an absolute time of zero is as valid as any other absolute value, so only
+   * a relative time value of zero resets the timer.
+   */
+  if (!use_abs_time && exp_time == 0) {
 	reset_kernel_timer(tp);
   } else {
-      tp->tmr_exp_time = (use_abs_time) ? exp_time : exp_time + get_monotonic();
+	if (!use_abs_time)
-      set_kernel_timer(tp, tp->tmr_exp_time, tp->tmr_func);
+		exp_time += uptime;
+	set_kernel_timer(tp, exp_time, cause_alarm, caller->p_endpoint);
   }
   return(OK);
 }
@@ -63,13 +66,12 @@ int do_setalarm(struct proc * caller, message * m_ptr)
 /*===========================================================================*
  *				cause_alarm				     *
  *===========================================================================*/
-static void cause_alarm(minix_timer_t *tp)
+static void cause_alarm(int proc_nr_e)
 {
 /* Routine called if a timer goes off and the process requested a synchronous
- * alarm. The process number is stored in timer argument 'ta_int'. Notify that
+ * alarm. The process number is stored as the timer argument. Notify that
  * process with a notification message from CLOCK.
  */
-  endpoint_t proc_nr_e = tmr_arg(tp)->ta_int;	/* get process number */
   mini_notify(proc_addr(CLOCK), proc_nr_e);	/* notify process */
 }
 

minix/kernel/utility.c

@@ -45,7 +45,7 @@ void panic(const char *fmt, ...)
 #endif
 
   /* Abort MINIX. */
-  minix_shutdown(NULL);
+  minix_shutdown(0);
 }
 
 /*===========================================================================*

minix/lib/libsys/sys_setalarm.c

@@ -2,8 +2,11 @@
 
 /*
  * Ask the kernel to schedule a synchronous alarm for the caller, using either
- * an absolute or a relative number of clock ticks.  Optionally return the time
+ * an absolute or a relative number of clock ticks.  The new alarm replaces any
- * left on the previous timer (TMR_NEVER if none was set) and the current time.
+ * previously set alarm.  If a relative expiry time of zero is given, the
+ * current alarm is stopped.  Return OK or a negative error code.  On success,
+ * optionally return the time left on the previous timer (TMR_NEVER if none was
+ * set) and the current time.
  */
 int
 sys_setalarm2(clock_t exp_time, int abs_time, clock_t * time_left,

minix/lib/libsys/timers.c

@@ -1,4 +1,5 @@
-/* Watchdog timer management. These functions in this file provide a
+/*
+ * Watchdog timer management. These functions in this file provide a
  * convenient interface to the timers library that manages a list of
  * watchdog timers. All details of scheduling an alarm at the CLOCK task
  * are hidden behind this interface.
@@ -16,71 +17,101 @@
 #include <minix/sysutil.h>
 
 static minix_timer_t *timers = NULL;
-static int expiring = 0;
+static int expiring = FALSE;
 
-/*===========================================================================*
+/*
- *                              init_timer                                   *
+ * Initialize the timer 'tp'.
- *===========================================================================*/
+ */
-void init_timer(minix_timer_t *tp)
+void
+init_timer(minix_timer_t * tp)
 {
+
 	tmr_inittimer(tp);
 }
 
-/*===========================================================================*
+/*
- *                              set_timer                                    *
+ * Set the timer 'tp' to trigger 'ticks' clock ticks in the future.  When it
- *===========================================================================*/
+ * triggers, call function 'watchdog' with argument 'arg'.  The given timer
-void set_timer(minix_timer_t *tp, int ticks, tmr_func_t watchdog, int arg)
+ * object must have been initialized with init_timer(3) already.  The given
+ * number of ticks must be between 0 and TMRDIFF_MAX inclusive.  A ticks value
+ * of zero will cause the alarm to trigger on the next clock tick.  If the
+ * timer was already set, it will be canceled first.
+ */
+void
+set_timer(minix_timer_t *tp, clock_t ticks, tmr_func_t watchdog, int arg)
 {
-        clock_t prev_time = 0, next_time;
+	clock_t prev_time, next_time;
+	int r, had_timers;
 
-        /* Set timer argument and add timer to the list. */
+	if (ticks > TMRDIFF_MAX)
-        tmr_arg(tp)->ta_int = arg;
+		panic("set_timer: ticks value too large: %u", (int)ticks);
-        prev_time = tmrs_settimer(&timers, tp, getticks() + ticks, watchdog,
+
-            &next_time);
+	/* Add the timer to the list. */
+	had_timers = tmrs_settimer(&timers, tp, getticks() + ticks, watchdog,
+	    arg, &prev_time, &next_time);
 
 	/* Reschedule our synchronous alarm if necessary. */
-        if (expiring == 0 && (! prev_time || prev_time > next_time)) {
+	if (!expiring && (!had_timers || next_time != prev_time)) {
-                if (sys_setalarm(next_time, 1) != OK)
+		if ((r = sys_setalarm(next_time, TRUE /*abs_time*/)) != OK)
-                        panic("set_timer: couldn't set alarm");
+			panic("set_timer: couldn't set alarm: %d", r);
         }
 }
 
-/*===========================================================================*
+/*
- *                              cancel_timer                                 *
+ * Cancel the timer 'tp'.  The timer object must have been initialized with
- *===========================================================================*/
+ * init_timer(3) first.  If the timer was not set before, the call is a no-op.
-void cancel_timer(minix_timer_t *tp)
+ */
+void
+cancel_timer(minix_timer_t * tp)
 {
 	clock_t next_time, prev_time;
-        prev_time = tmrs_clrtimer(&timers, tp, &next_time);
+	int r, have_timers;
 
-        /* If the earliest timer has been removed, we have to set the alarm to
+	if (!tmr_is_set(tp))
+		return;
+
+	have_timers = tmrs_clrtimer(&timers, tp, &prev_time, &next_time);
+
+	/*
+	 * If the earliest timer has been removed, we have to set the alarm to
 	 * the next timer, or cancel the alarm altogether if the last timer
-         * has been cancelled (next_time will be 0 then).
+	 * has been canceled.
 	 */
-        if (expiring == 0 && (prev_time < next_time || ! next_time)) {
+        if (!expiring) {
-                if (sys_setalarm(next_time, 1) != OK)
+		if (!have_timers)
-                        panic("cancel_timer: couldn't set alarm");
+			r = sys_setalarm(0, FALSE /*abs_time*/);
+		else if (prev_time != next_time)
+			r = sys_setalarm(next_time, TRUE /*abs_time*/);
+		else
+			r = OK;
+
+		if (r != OK)
+                        panic("cancel_timer: couldn't set alarm: %d", r);
         }
 }
 
-/*===========================================================================*
+/*
- *                              expire_timers                                *
+ * Expire all timers that were set to expire before/at the given current time.
- *===========================================================================*/
+ */
-void expire_timers(clock_t now)
+void
+expire_timers(clock_t now)
 {
         clock_t next_time;
+	int r, have_timers;
 
-        /* Check for expired timers. Use a global variable to indicate that
+	/*
+	 * Check for expired timers. Use a global variable to indicate that
 	 * watchdog functions are called, so that sys_setalarm() isn't called
 	 * more often than necessary when set_timer or cancel_timer are called
-         * from these watchdog functions. */
+	 * from these watchdog functions.
-        expiring = 1;
+	 */
-        tmrs_exptimers(&timers, now, &next_time);
+	expiring = TRUE;
-        expiring = 0;
+	have_timers = tmrs_exptimers(&timers, now, &next_time);
+	expiring = FALSE;
 
 	/* Reschedule an alarm if necessary. */
-        if (next_time > 0) {
+	if (have_timers) {
-                if (sys_setalarm(next_time, 1) != OK)
+		if ((r = sys_setalarm(next_time, TRUE /*abs_time*/)) != OK)
-                        panic("expire_timers: couldn't set alarm");
+			panic("expire_timers: couldn't set alarm: %d", r);
 	}
 }

minix/lib/libtimers/timers.h

@@ -1,6 +0,0 @@
-/* This library provides generic watchdog timer management functionality.
- * See the comments in <minix/timers.h> for details.
- */
-
-#include <minix/timers.h>		/* definitions and function prototypes */
-#include <sys/null.h>

minix/lib/libtimers/tmrs_clr.c

@@ -1,24 +1,30 @@
-#include "timers.h"
+#include <minix/timers.h>
 
-/*===========================================================================*
+/*
- *				tmrs_clrtimer				     *
+ * Deactivate a timer and remove it from the timers queue.  'tmrs' is a pointer
- *===========================================================================*/
+ * to the timers queue.  'tp' is a pointer to the timer to be removed, which
-clock_t tmrs_clrtimer(tmrs, tp, next_time)
+ * generally should be on the queue (but this is not a requirement, and the
-minix_timer_t **tmrs;				/* pointer to timers queue */
+ * kernel abuses this).  If 'prev_time' is non-NULL, it is filled with the
-minix_timer_t *tp;				/* timer to be removed */
+ * previous timer head time, which always exists since at least 'tp' is on it.
-clock_t *next_time;
+ * The function returns TRUE if there is still at least one timer on the queue
-{
+ * after this function is done, in which case 'next_time' (if non-NULL) is
-/* Deactivate a timer and remove it from the timers queue. 
+ * filled with the absolute expiry time of the new head timer.
  */
+int
+tmrs_clrtimer(minix_timer_t ** tmrs, minix_timer_t * tp, clock_t * prev_time,
+	clock_t * next_time)
+{
 	minix_timer_t **atp;
-  clock_t prev_time;
+	int r;
 
-  if(*tmrs)
+	if (*tmrs != NULL) {
-  	prev_time = (*tmrs)->tmr_exp_time;
+		if (prev_time != NULL)
-  else
+			*prev_time = (*tmrs)->tmr_exp_time;
-  	prev_time = 0;
+		r = TRUE;
+	} else
+		r = FALSE;
 
-  tp->tmr_exp_time = TMR_NEVER;
+	tp->tmr_func = NULL;	/* clear the timer object */
 
 	for (atp = tmrs; *atp != NULL; atp = &(*atp)->tmr_next) {
 		if (*atp == tp) {
@@ -27,13 +33,12 @@ clock_t *next_time;
 		}
 	}
 
-  if(next_time) {
+	if (next_time != NULL) {
-  	if(*tmrs)
+		if (*tmrs != NULL)
 			*next_time = (*tmrs)->tmr_exp_time;
 		else
 			*next_time = 0;
 	}
 
-  return prev_time;
+	return r;
 }
-

minix/lib/libtimers/tmrs_exp.c

@@ -1,31 +1,29 @@
-#include "timers.h"
+#include <minix/timers.h>
 
-/*===========================================================================*
+/*
- *				tmrs_exptimers				     *
+ * Use the current time to check the timers queue list for expired timers.
- *===========================================================================*/
-void tmrs_exptimers(tmrs, now, new_head)
-minix_timer_t **tmrs;				/* pointer to timers queue */
-clock_t now;				/* current time */
-clock_t *new_head;
-{
-/* Use the current time to check the timers queue list for expired timers. 
  * Run the watchdog functions for all expired timers and deactivate them.
  * The caller is responsible for scheduling a new alarm if needed.
  */
+int
+tmrs_exptimers(minix_timer_t ** tmrs, clock_t now, clock_t * new_head)
+{
 	minix_timer_t *tp;
+	tmr_func_t func;
 
-  while ((tp = *tmrs) != NULL && tp->tmr_exp_time <= now) {
+	while ((tp = *tmrs) != NULL && tmr_has_expired(tp, now)) {
 		*tmrs = tp->tmr_next;
-	tp->tmr_exp_time = TMR_NEVER;
+
-	(*tp->tmr_func)(tp);
+		func = tp->tmr_func;
+		tp->tmr_func = NULL;
+
+		(*func)(tp->tmr_arg);
 	}
 
-  if(new_head) {
+	if (*tmrs != NULL) {
-  	if(*tmrs)
+		if (new_head != NULL)
 			*new_head = (*tmrs)->tmr_exp_time;
-  	else
+		return TRUE;
-  		*new_head = 0;
+	} else
-  }
+		return FALSE;
 }
-
-

minix/lib/libtimers/tmrs_set.c

@@ -1,39 +1,48 @@
-#include "timers.h"
+#include <minix/timers.h>
 
-/*===========================================================================*
+/*
- *				tmrs_settimer				     *
+ * Activate a timer to run function 'watchdog' at absolute time 'exp_time', as
- *===========================================================================*/
+ * part of timers queue 'tmrs'. If the timer is already in use, it is first
-clock_t tmrs_settimer(tmrs, tp, exp_time, watchdog, new_head)
+ * removed from the timers queue.  Then, it is put in the list of active timers
-minix_timer_t **tmrs;				/* pointer to timers queue */
+ * with the first to expire in front.  The caller responsible for scheduling a
-minix_timer_t *tp;				/* the timer to be added */
+ * new alarm for the timer if needed.  To that end, the function returns three
-clock_t exp_time;			/* its expiration time */
+ * values: its return value (TRUE or FALSE) indicates whether there was an old
-tmr_func_t watchdog;			/* watchdog function to be run */
+ * head timer; if TRUE, 'old_head' (if non-NULL) is filled with the absolute
-clock_t *new_head;			/* new earliest timer, if non NULL */
+ * expiry time of the old head timer.  If 'new_head' is non-NULL, it is filled
-{
+ * with the absolute expiry time of the new head timer.
-/* Activate a timer to run function 'fp' at time 'exp_time'. If the timer is
- * already in use it is first removed from the timers queue. Then, it is put
- * in the list of active timers with the first to expire in front.
- * The caller responsible for scheduling a new alarm for the timer if needed. 
  */
+int
+tmrs_settimer(minix_timer_t ** tmrs, minix_timer_t * tp, clock_t exp_time,
+	tmr_func_t watchdog, int arg, clock_t * old_head, clock_t * new_head)
+{
 	minix_timer_t **atp;
-  clock_t old_head = 0;
+	int r;
 
-  if(*tmrs)
+	if (*tmrs != NULL) {
-  	old_head = (*tmrs)->tmr_exp_time;
+		if (old_head != NULL)
+			*old_head = (*tmrs)->tmr_exp_time;
+		r = TRUE;
+	} else
+		r = FALSE;
 
 	/* Set the timer's variables. */
-  (void) tmrs_clrtimer(tmrs, tp, NULL);
+	if (tmr_is_set(tp))
+		(void)tmrs_clrtimer(tmrs, tp, NULL, NULL);
 	tp->tmr_exp_time = exp_time;
-  tp->tmr_func = watchdog;
+	tp->tmr_func = watchdog;	/* set the timer object */
+	tp->tmr_arg = arg;
 
-  /* Add the timer to the active timers. The next timer due is in front. */
+	/*
+	 * Add the timer to the active timers. The next timer due is in front.
+	 */
 	for (atp = tmrs; *atp != NULL; atp = &(*atp)->tmr_next) {
-	if (exp_time < (*atp)->tmr_exp_time) break;
+		if (tmr_is_first(exp_time, (*atp)->tmr_exp_time))
+			break;
 	}
 	tp->tmr_next = *atp;
 	*atp = tp;
-  if(new_head)
-  	(*new_head) = (*tmrs)->tmr_exp_time;
-  return old_head;
-}
 
+	if (new_head != NULL)
+		*new_head = (*tmrs)->tmr_exp_time;
+	return r;
+}

minix/net/lwip/lwip.c

@@ -36,19 +36,19 @@ static void sys_init(void)
 {
 }
 
-static void arp_watchdog(__unused minix_timer_t *tp)
+static void arp_watchdog(int arg __unused)
 {
 	etharp_tmr();
 	set_timer(&arp_tmr, arp_ticks, arp_watchdog, 0);
 }
 
-static void tcp_fwatchdog(__unused minix_timer_t *tp)
+static void tcp_fwatchdog(int arg __unused)
 {
 	tcp_fasttmr();
 	set_timer(&tcp_ftmr, tcp_fticks, tcp_fwatchdog, 0);
 }
 
-static void tcp_swatchdog(__unused minix_timer_t *tp)
+static void tcp_swatchdog(int arg __unused)
 {
 	tcp_slowtmr();
 	set_timer(&tcp_ftmr, tcp_sticks, tcp_swatchdog, 0);

minix/servers/pm/alarm.c

@@ -13,6 +13,7 @@
 #include <sys/time.h>
 #include <minix/com.h>
 #include <minix/callnr.h>
+#include <assert.h>
 #include "mproc.h"
 
 #define US 1000000UL	/* shortcut for microseconds per second */
@@ -24,7 +25,7 @@ static void getset_vtimer(struct mproc *mp, int nwhich, struct
 	itimerval *value, struct itimerval *ovalue);
 static void get_realtimer(struct mproc *mp, struct itimerval *value);
 static void set_realtimer(struct mproc *mp, struct itimerval *value);
-static void cause_sigalrm(minix_timer_t *tp);
+static void cause_sigalrm(int arg);
 
 /*===========================================================================*
  *				ticks_from_timeval			     *
@@ -252,7 +253,7 @@ get_realtimer(struct mproc *rmp, struct itimerval *value)
   /* First determine remaining time, in ticks, of previous alarm, if set. */
   if (rmp->mp_flags & ALARM_ON) {
 	uptime = getticks();
-  	exptime = *tmr_exp_time(&rmp->mp_timer);
+	exptime = tmr_exp_time(&rmp->mp_timer);
 
   	remaining = exptime - uptime;
 
@@ -300,6 +301,7 @@ struct mproc *rmp;		/* process that wants the alarm */
 clock_t ticks;			/* how many ticks delay before the signal */
 {
   if (ticks > 0) {
+	assert(ticks <= TMRDIFF_MAX);
   	set_timer(&rmp->mp_timer, ticks, cause_sigalrm, rmp->mp_endpoint);
 	rmp->mp_flags |=  ALARM_ON;
   } else if (rmp->mp_flags & ALARM_ON) {
@@ -311,16 +313,15 @@ clock_t ticks;			/* how many ticks delay before the signal */
 /*===========================================================================*
  *				cause_sigalrm				     *
  *===========================================================================*/
-static void cause_sigalrm(tp)
+static void
-minix_timer_t *tp;
+cause_sigalrm(int arg)
 {
   int proc_nr_n;
   register struct mproc *rmp;
 
   /* get process from timer */
-  if(pm_isokendpt(tmr_arg(tp)->ta_int, &proc_nr_n) != OK) {
+  if(pm_isokendpt(arg, &proc_nr_n) != OK) {
-  	printf("PM: ignoring timer for invalid endpoint %d\n",
+	printf("PM: ignoring timer for invalid endpoint %d\n", arg);
-  		tmr_arg(tp)->ta_int);
   	return;
   }
 
@@ -331,8 +332,7 @@ minix_timer_t *tp;
 
   /* If an interval is set, set a new timer; otherwise clear the ALARM_ON flag.
    * The set_alarm call will be calling set_timer from within this callback
-   * from the expire_timers function. This is safe, but we must not use the
+   * from the expire_timers function. This is safe.
-   * "tp" structure below this point anymore.
    */
   if (rmp->mp_interval[ITIMER_REAL] > 0)
 	set_alarm(rmp, rmp->mp_interval[ITIMER_REAL]);

minix/servers/pm/const.h

@@ -12,8 +12,7 @@
 
 #define NO_EVENTSUB	((char)-1) /* no current process event subscriber */
 
-#define MAX_CLOCK_T	((unsigned long) 1 << ((sizeof(clock_t) * 8) - 1))
+#define MAX_SECS	( (clock_t) (TMRDIFF_MAX/system_hz) )
-#define MAX_SECS	( (clock_t) (MAX_CLOCK_T/system_hz) )
 				/* max.secs for setitimer() ((2^31-1)/HZ) */
 #define NR_ITIMERS	   3	/* number of supported interval timers */
 

minix/servers/vfs/proto.h

@@ -351,7 +351,6 @@ void select_callback(struct filp *, int ops);
 void select_forget(void);
 void select_reply1(endpoint_t driver_e, devminor_t minor, int status);
 void select_reply2(endpoint_t driver_e, devminor_t minor, int status);
-void select_timeout_check(minix_timer_t *);
 void select_unsuspend_by_endpt(endpoint_t proc);
 
 /* worker.c */

minix/servers/vfs/select.c

@@ -32,6 +32,8 @@
 #define FROM_PROC 0
 #define TO_PROC   1
 
+#define USECPERSEC 1000000	/* number of microseconds in a second */
+
 typedef fd_set *ixfer_fd_set_ptr;
 
 static struct selectentry {
@@ -71,6 +73,7 @@ static void select_return(struct selectentry *);
 static void select_restart_filps(void);
 static int tab2ops(int fd, struct selectentry *e);
 static void wipe_select(struct selectentry *s);
+void select_timeout_check(int s);
 
 static struct fdtype {
 	int (*select_request)(struct filp *, int *ops, int block,
@@ -96,12 +99,13 @@ int do_select(void)
  * timeout and wait for either the file descriptors to become ready or the
  * timer to go off. If no timeout value was provided, we wait indefinitely.
  */
-  int r, nfds, do_timeout = 0, fd, s;
+  int r, nfds, do_timeout, fd, s;
   struct filp *f;
   unsigned int type, ops;
   struct timeval timeout;
   struct selectentry *se;
   vir_bytes vtimeout;
+  clock_t ticks;
 
   nfds = job_m_in.m_lc_vfs_select.nfds;
   vtimeout = job_m_in.m_lc_vfs_select.timeout;
@@ -131,20 +135,21 @@ int do_select(void)
 
   /* Did the process set a timeout value? If so, retrieve it. */
   if (vtimeout != 0) {
-	do_timeout = 1;
 	r = sys_datacopy_wrapper(who_e, vtimeout, SELF, (vir_bytes) &timeout,
 		sizeof(timeout));
+
+	/* No nonsense in the timeval */
+	if (r == OK && (timeout.tv_sec < 0 || timeout.tv_usec < 0 ||
+	    timeout.tv_usec >= USECPERSEC))
+		r = EINVAL;
+
 	if (r != OK) {
 		se->requestor = NULL;
 		return(r);
 	}
-  }
+	do_timeout = 1;
-
+  } else
-  /* No nonsense in the timeval */
+	do_timeout = 0;
-  if (do_timeout && (timeout.tv_sec < 0 || timeout.tv_usec < 0)) {
-	se->requestor = NULL;
-	return(EINVAL);
-  }
 
   /* If there is no timeout, we block forever. Otherwise, we block up to the
    * specified time interval.
@@ -282,22 +287,20 @@ int do_select(void)
   /* Convert timeval to ticks and set the timer. If it fails, undo
    * all, return error.
    */
-  if (do_timeout) {
+  if (do_timeout && se->block) {
-	int ticks;
 	/* Open Group:
 	 * "If the requested timeout interval requires a finer
 	 * granularity than the implementation supports, the
 	 * actual timeout interval shall be rounded up to the next
 	 * supported value."
 	 */
-#define USECPERSEC 1000000
+	if (timeout.tv_sec >= (TMRDIFF_MAX - 1) / system_hz) {
-	while(timeout.tv_usec >= USECPERSEC) {
+		ticks = TMRDIFF_MAX; /* silently truncate */
-		/* this is to avoid overflow with *system_hz below */
+	} else {
-		timeout.tv_usec -= USECPERSEC;
-		timeout.tv_sec++;
-	}
 		ticks = timeout.tv_sec * system_hz +
 		    (timeout.tv_usec * system_hz + USECPERSEC-1) / USECPERSEC;
+	}
+	assert(ticks != 0 && ticks <= TMRDIFF_MAX);
 	se->expiry = ticks;
 	set_timer(&se->timer, ticks, select_timeout_check, s);
   }
@@ -742,20 +745,18 @@ void select_forget(void)
 /*===========================================================================*
  *				select_timeout_check	  	     	     *
  *===========================================================================*/
-void select_timeout_check(minix_timer_t *timer)
+void select_timeout_check(int s)
 {
 /* An alarm has gone off for one of the select queries. This function MUST NOT
  * block its calling thread.
  */
-  int s;
   struct selectentry *se;
 
-  s = tmr_arg(timer)->ta_int;
   if (s < 0 || s >= MAXSELECTS) return;	/* Entry does not exist */
 
   se = &selecttab[s];
   if (se->requestor == NULL) return;
-  if (se->expiry <= 0) return;	/* Strange, did we even ask for a timeout? */
+  if (se->expiry == 0) return;	/* Strange, did we even ask for a timeout? */
   se->expiry = 0;
   if (!is_deferred(se))
 	select_return(se);

minix/tests/t40f.c

@@ -59,7 +59,7 @@ static void do_child(void) {
   struct timeval tv;
  
   /* Let the parent do initial read and write tests from and to the pipe. */
-  tv.tv_sec = DO_PAUSE + DO_PAUSE + DO_PAUSE + 1;
+  tv.tv_sec = DO_PAUSE + DO_PAUSE + 1;
   tv.tv_usec = 0;
   (void) select(0, NULL, NULL, NULL, &tv);
 
@@ -109,16 +109,28 @@ static void do_parent(int child) {
   retval = select(fd_ap[0]+1, &fds_read, NULL, NULL, &tv); 
   (void) gettimeofday(&end_time, NULL);     /* Record ending time */
   
-  if(retval != 0) em(3, "Should have timed out");
+  if(retval != -1) em(3, "Should have failed");
-  if(compute_diff(start_time, end_time, DO_PAUSE) > DO_DELTA)
+  if(errno != EINVAL) em(4, "Incorrect error thrown");
-    em(4, "Time difference too large");
+
+  /* Do a few more tests for invalid timeout values. */
+  tv.tv_sec = 0;
+  tv.tv_usec = 1000000;
+  retval = select(fd_ap[0]+1, &fds_read, NULL, NULL, &tv);
+  if (retval != -1) em(0, "Should have failed");
+  if (errno != EINVAL) em(0, "Incorrect error thrown");
+
+  tv.tv_sec = 0;
+  tv.tv_usec = ~0;
+  retval = select(fd_ap[0]+1, &fds_read, NULL, NULL, &tv);
+  if (retval != -1) em(0, "Should have failed");
+  if (errno != EINVAL) em(0, "Incorrect error thrown");
 
   /* Let's wait for another DO_PAUSE seconds, expressed in seconds and micro
      seconds. */
   FD_ZERO(&fds_read);
   FD_SET(fd_ap[0], &fds_read);
   tv.tv_sec = DO_PAUSE - 1;
-  tv.tv_usec = (DO_PAUSE - tv.tv_sec) * 1000000L;
+  tv.tv_usec = 999999L; /* close enough */
   
   (void) gettimeofday(&start_time, NULL);   /* Record starting time */
   retval = select(fd_ap[0]+1, &fds_read, NULL, NULL, &tv);