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");
