> Date: Wed, 14 Oct 2020 20:14:18 -0500 > From: Scott Cheloha <scottchel...@gmail.com> > > On Wed, Oct 14, 2020 at 08:06:52PM -0500, Scott Cheloha wrote: > > _exit(2) and execve(2) need to obey the locking protocol described in > > proc.h when manipulating the per-process interval timer state. > > > > While we're here we can also remove the now pointless splclock/splx > > dance from execve(2). > > > > The easiest way to obey the locking protocol is to reuse the interface > > the syscalls are using: setitimer() in kern_time.c. > > > > Given that we only want to cancel the timers I wrote a small helper > > function, cancelitimer(). I think it's tidier than putting the > > prototype for setitimer() into sys/time.h and requiring the caller to > > prepare an itimerval struct before calling. > > > > Compare: > > > > struct itimerval itv; > > timerclear(&itv.it_value); > > timerclear(&itv.it_interval); > > setitimer(ITIMER_REAL, &itv, NULL); > > > > with: > > > > cancelitimer(ITIMER_REAL); > > > > ... should I shove the for-loop into the helper function too? Maybe > > call it "cancel_all_itimers()"? I have a vague feeling that showing > > the reader that there are multiple timers is a good thing here, but > > then again maybe I'm wrong and nobody cares. > > > > Preferences? ok? > > Whoops, forgot the kern_time.c part of the diff. > > Index: kern/kern_exit.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_exit.c,v > retrieving revision 1.188 > diff -u -p -r1.188 kern_exit.c > --- kern/kern_exit.c 18 Mar 2020 15:48:21 -0000 1.188 > +++ kern/kern_exit.c 15 Oct 2020 01:12:50 -0000 > @@ -194,7 +194,11 @@ exit1(struct proc *p, int xexit, int xsi > /* close open files and release open-file table */ > fdfree(p); > > - timeout_del(&pr->ps_realit_to); > + /* cancel all interval timers */ > + int i;
Don't put variable definitions in the middle of a function. > + for (i = 0; i < nitems(pr->ps_timer); i++) > + cancelitimer(i); > + > timeout_del(&pr->ps_rucheck_to); > #ifdef SYSVSEM > semexit(pr); > Index: kern/kern_exec.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_exec.c,v > retrieving revision 1.217 > diff -u -p -r1.217 kern_exec.c > --- kern/kern_exec.c 11 Jul 2020 22:59:05 -0000 1.217 > +++ kern/kern_exec.c 15 Oct 2020 01:12:50 -0000 > @@ -656,14 +656,9 @@ sys_execve(struct proc *p, void *v, regi > } > > if (pr->ps_flags & PS_SUGIDEXEC) { > - int i, s = splclock(); > - > - timeout_del(&pr->ps_realit_to); > - for (i = 0; i < nitems(pr->ps_timer); i++) { > - timespecclear(&pr->ps_timer[i].it_interval); > - timespecclear(&pr->ps_timer[i].it_value); > - } > - splx(s); > + int i; You should put a blank line here to separate definitions from the code. > + for (i = 0; i < nitems(pr->ps_timer); i++) > + cancelitimer(i); > } > > /* reset CPU time usage for the thread, but not the process */ > Index: kern/kern_time.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_time.c,v > retrieving revision 1.146 > diff -u -p -r1.146 kern_time.c > --- kern/kern_time.c 13 Oct 2020 17:33:39 -0000 1.146 > +++ kern/kern_time.c 15 Oct 2020 01:12:50 -0000 > @@ -572,6 +572,16 @@ setitimer(int which, const struct itimer > } > } > > +void > +cancelitimer(int which) > +{ > + struct itimerval itv; > + > + timerclear(&itv.it_value); > + timerclear(&itv.it_interval); > + setitimer(which, &itv, NULL); > +} > + > int > sys_getitimer(struct proc *p, void *v, register_t *retval) > { > Index: sys/time.h > =================================================================== > RCS file: /cvs/src/sys/sys/time.h,v > retrieving revision 1.55 > diff -u -p -r1.55 time.h > --- sys/time.h 6 Jul 2020 13:33:09 -0000 1.55 > +++ sys/time.h 15 Oct 2020 01:12:50 -0000 > @@ -307,6 +307,7 @@ time_t getuptime(void); > struct proc; > int clock_gettime(struct proc *, clockid_t, struct timespec *); > > +void cancelitimer(int); > int itimerfix(struct timeval *); > int itimerdecr(struct itimerspec *, long); > int settime(const struct timespec *); > >