[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: shutdown: simplify countdown loop()
From:       Scott Cheloha <scottcheloha () gmail ! com>
Date:       2018-02-25 22:51:22
Message-ID: 20180225225122.fjufvchqmk26nobo () rascal ! austin ! ibm ! com
[Download RAW message or body]

On Sun, Feb 25, 2018 at 10:42:04AM +0100, Theo Buehler wrote:
> On Sat, Feb 24, 2018 at 06:25:44PM -0600, Scott Cheloha wrote:
> > [...]
> > 
> > If we treat tlist[] like an array instead of a list, we can
> > eliminate a lot of special cases and duplicate calls in shutdown(8)'s
> > countdown loop(). [...]
> > 
> > ok?
> 
> Yes, I think that's easier to follow. Two questions below.
>
> > 
> > Index: sbin/shutdown/shutdown.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
> > retrieving revision 1.48
> > diff -u -p -r1.48 shutdown.c
> > --- sbin/shutdown/shutdown.c	24 Feb 2018 20:00:07 -0000	1.48
> > +++ sbin/shutdown/shutdown.c	24 Feb 2018 23:57:54 -0000
> > @@ -64,8 +64,10 @@
> >  #define	S		*1
> >  #define	NOLOG_TIME	5*60
> >  struct interval {
> > -	int timeleft, timetowait;
> > +	time_t timeleft;
> > +	time_t timetowait;
> 
> Should we suffix a constant with LL in the multipliers H, M, S so
> we get correct overflow handling in the initializations below (not
> currently an issue).

If I'm understanding you correctly, no.  We're nowhere near INT_MAX,
and these decimal constants are ints as they lack the suffix.

And even if we did something crazy, like add an interval like this:

	{ 596524 H, 0 }

which just barely overflows a 32-bit type, on amd64 clang gives me:

	cc -O2 -pipe  -Werror-implicit-function-declaration -MD -MP  -c /usr/src/sbin/shutdown/shutdown.c
	/usr/src/sbin/shutdown/shutdown.c:70:11: warning: overflow in expression; result
	      is -2147480896 with type 'int' [-Winteger-overflow]
	        { 596524 H,    0 },
	                 ^
	/usr/src/sbin/shutdown/shutdown.c:62:15: note: expanded from macro 'H'
	#define H               *60*60
	                           ^
	1 warning generated.
	cc  -static -pie -o shutdown shutdown.o 

and on macppc, gcc gives me:

	cc -O2 -pipe  -Werror-implicit-function-declaration -MD -MP  -c /usr/src/sbin/shutdown/shutdown.c
	/usr/src/sbin/shutdown/shutdown.c:69: warning: integer overflow in expression
	cc  -static -pie -o shutdown shutdown.o 

which, while not as glaringly obvious as clang's warning, is
sufficient, I think.

> > [...]
> > @@ -273,7 +274,7 @@ static char *restricted_environ[] = {
> >  };
> >  
> >  void
> > -timewarn(int timeleft)
> > +timewarn(time_t timeleft)
> >  {
> >  	static char hostname[HOST_NAME_MAX+1];
> >  	static int first;
> > @@ -321,7 +322,7 @@ timewarn(int timeleft)
> >  		    tm->tm_hour, tm->tm_min);
> >  	} else if (timeleft > 59)
> >  		dprintf(fd[1], "System going down in %d minute%s\n\n",
> 
> Wouldn't it be better to use an %lld format and leave the next line as
> it is?

Yes, we ought to use %lld, if only because code tends to move around.

But we can't leave the expression uncasted.  time_t has no dedicated
format specifier so we need an explicit cast.  At least, I've never
seen one in base printed without a cast.  And I suspect that such code
wouldn't be portable.

Updated diff attached.

--
Scott Cheloha

Index: sbin/shutdown/shutdown.c
===================================================================
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.48
diff -u -p -r1.48 shutdown.c
--- sbin/shutdown/shutdown.c	24 Feb 2018 20:00:07 -0000	1.48
+++ sbin/shutdown/shutdown.c	25 Feb 2018 22:44:39 -0000
@@ -64,8 +64,10 @@
 #define	S		*1
 #define	NOLOG_TIME	5*60
 struct interval {
-	int timeleft, timetowait;
+	time_t timeleft;
+	time_t timetowait;
 } tlist[] = {
+	{    0,    0 },
 	{ 10 H,  5 H },
 	{  5 H,  3 H },
 	{  2 H,  1 H },
@@ -79,6 +81,7 @@ struct interval {
 	{ 30 S, 30 S },
 	{    0,    0 }
 };
+const int tlistlen = sizeof(tlist) / sizeof(tlist[0]);
 #undef H
 #undef M
 #undef S
@@ -96,7 +99,7 @@ void getoffset(char *);
 void __dead loop(void);
 void nolog(void);
 void timeout(int);
-void timewarn(int);
+void timewarn(time_t);
 void usage(void);
 
 int
@@ -229,40 +232,38 @@ main(int argc, char *argv[])
 void
 loop(void)
 {
-	struct interval *tp;
-	u_int sltime;
-	int logged;
-
-	if (offset <= NOLOG_TIME) {
-		logged = 1;
-		nolog();
-	} else
-		logged = 0;
-	tp = tlist;
-	if (tp->timeleft < offset)
-		(void)sleep((u_int)(offset - tp->timeleft));
-	else {
-		while (offset < tp->timeleft)
-			++tp;
-		/*
-		 * Warn now, if going to sleep more than a fifth of
-		 * the next wait time.
-		 */
-		if ((sltime = offset - tp->timeleft)) {
-			if (sltime > tp->timetowait / 5)
-				timewarn(offset);
-			(void)sleep(sltime);
+	struct timespec timeout;
+	int broadcast, i, logged;
+
+	broadcast = 1;
+
+	for (i = 0; i < tlistlen - 1; i++) {
+		if (offset > tlist[i + 1].timeleft) {
+			tlist[i].timeleft = offset;
+			tlist[i].timetowait = offset - tlist[i + 1].timeleft;
+			break;
 		}
 	}
-	for (;; ++tp) {
-		timewarn(tp->timeleft);
-		if (!logged && tp->timeleft <= NOLOG_TIME) {
+
+	/*
+	 * Don't spam the users: skip our offset's warning broadcast if
+	 * there's a broadcast scheduled after ours and it's relatively
+	 * imminent.
+	 */
+	if (offset > 0 && tlist[i].timetowait < tlist[i + 1].timetowait / 5)
+		broadcast = 0;
+
+	for (logged = 0; i < tlistlen; i++) {
+		if (broadcast)
+			timewarn(tlist[i].timeleft);
+		broadcast = 1;
+		if (!logged && tlist[i].timeleft <= NOLOG_TIME) {
 			logged = 1;
 			nolog();
 		}
-		(void)sleep((u_int)tp->timetowait);
-		if (!tp->timeleft)
-			break;
+		timeout.tv_sec = tlist[i].timetowait;
+		timeout.tv_nsec = 0;
+		nanosleep(&timeout, NULL);
 	}
 	die_you_gravy_sucking_pig_dog();
 }
@@ -273,7 +274,7 @@ static char *restricted_environ[] = {
 };
 
 void
-timewarn(int timeleft)
+timewarn(time_t timeleft)
 {
 	static char hostname[HOST_NAME_MAX+1];
 	static int first;
@@ -319,10 +320,10 @@ timewarn(int timeleft)
 
 		dprintf(fd[1], "System going down at %d:%02d\n\n",
 		    tm->tm_hour, tm->tm_min);
-	} else if (timeleft > 59)
-		dprintf(fd[1], "System going down in %d minute%s\n\n",
-		    timeleft / 60, (timeleft > 60) ? "s" : "");
-	else if (timeleft)
+	} else if (timeleft > 59) {
+		dprintf(fd[1], "System going down in %lld minute%s\n\n",
+		    (long long)(timeleft / 60), (timeleft > 60) ? "s" : "");
+	} else if (timeleft)
 		dprintf(fd[1], "System going down in 30 seconds\n\n");
 	else
 		dprintf(fd[1], "System going down IMMEDIATELY\n\n");

[prev in list] [next in list] [prev in thread] [next in thread]