From owner-freebsd-standards Fri Dec 28 0:17:56 2001 Delivered-To: freebsd-standards@freebsd.org Received: from espresso.q9media.com (espresso.q9media.com [216.254.138.122]) by hub.freebsd.org (Postfix) with ESMTP id 5181337B416 for ; Fri, 28 Dec 2001 00:17:46 -0800 (PST) Received: (from mike@localhost) by espresso.q9media.com (8.11.6/8.11.6) id fBS8Fbf23518; Fri, 28 Dec 2001 03:15:37 -0500 (EST) (envelope-from mike) Date: Fri, 28 Dec 2001 03:15:37 -0500 From: Mike Barcroft To: Joe Halpin Cc: "standards@FreeBSD.ORG" Subject: Re: at utility changes Message-ID: <20011228031537.B99161@espresso.q9media.com> References: <3C200BBA.9D26ED93@attbi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3C200BBA.9D26ED93@attbi.com>; from joe.halpin@attbi.com on Tue, Dec 18, 2001 at 09:38:34PM -0600 Organization: The FreeBSD Project Sender: owner-freebsd-standards@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Joe Halpin writes: > I'm attaching the diff file for the changes I made to the at utility, in > order to add the -r and -t options. Thanks to Mike for his help in > getting my head into the procedures. Sorry for the delay in my review. > Index: at/at.c > =================================================================== > RCS file: /home/ncvs/src/usr.bin/at/at.c,v > retrieving revision 1.23 > diff -u -r1.23 at.c > --- at/at.c 2001/12/10 21:13:01 1.23 > +++ at/at.c 2001/12/19 03:30:02 > @@ -121,6 +121,8 @@ > static void writefile(time_t runtimer, char queue); > static void list_jobs(void); > static long nextjob(void); > +static time_t ttime(const char *argtime); > +static inline int checkint(const char *numstr); > > /* Signal catching functions */ > > @@ -592,6 +594,134 @@ > } > } /* delete_jobs */ > > + Please remove the extra vertical space here. > +static inline int checkint(const char *numstr) > +{ > + if((!isdigit(numstr[0])) || (!isdigit(numstr[1]))) > + panic("non-numeric character(s) in date"); > + > + return (int) strtol(numstr, 0, 10); > +} This file has fairly inconsistent style which makes it hard to read. I would suggest you try to follow style(9), with the exception of tabbing. This means `if(...)' should become `if (...)', `return ...;' should become `return (...);'. The tabbing appears to be 4 space, then a tab for further indentation. I would recommend following this in your patch. Additionally, there are some gratuitous parens around `!isdigit(...)' here. > + > +static time_t ttime(const char *argtime) > +{ > + /* > + * -t format: [[CC]YY]MMDDhhmm[.SS] > + */ > + > + time_t nowtimer, runtimer; > + struct tm nowtime, runtime; > + int arglen; > + char *cp; > + char workstr[16]; > + int C = -1; > + int Y = -1; > + int M = -1; > + int D = -1; > + int h = -1; > + int m = -1; > + int s = 0; > + > + arglen = strlen(argtime); > + if(arglen < 8 || arglen > 15) > + panic("Invalid time format"); > + > + strcpy(workstr, argtime); > + if((cp = strchr(workstr, '.')) != NULL) > + ++cp; > + > + if(cp) { > + if(arglen < 11) > + panic("invalid time format"); > + > + s = checkint(cp); *(--cp) = 0; cp -= 2; > + m = checkint(cp); *cp = 0; cp -= 2; > + h = checkint(cp); *cp = 0; cp -= 2; > + D = checkint(cp); *cp = 0; cp -= 2; > + M = checkint(cp); *cp = 0; > + > + if(arglen == 12) > + panic("invalid time format"); > + > + if(arglen >= 13) { > + cp -= 2; > + Y = checkint(cp); > + *cp = 0; > + } > + > + if(arglen == 14) > + panic("invalid time format"); > + > + if(arglen == 15) { > + cp -= 2; > + C = checkint(cp); > + } > + } > + > + else { > + if(arglen < 8) > + panic("invalid date format"); > + > + cp = workstr + arglen - 2; > + m = checkint(cp); *cp = 0; cp -= 2; > + h = checkint(cp); *cp = 0; cp -= 2; > + D = checkint(cp); *cp = 0; cp -= 2; > + M = checkint(cp); *cp = 0; > + > + if(arglen == 9) > + panic("invalid time format"); > + > + if(arglen >= 10) { > + cp -= 2; > + Y = checkint(cp); > + *cp = 0; > + } > + > + if(arglen == 11) > + panic("invalid time format"); > + > + if(arglen == 12) { > + cp -= 2; > + C = checkint(cp); > + } > + } > + > + if(Y > -1) { > + /* Since we're past Y2K now, add 100 to the year (or whatever the > + * century given is). > + */ > + if(C > -1) > + Y = ((C * 100) + Y) - 1900; > + else > + Y += 100; > + } > + > + nowtimer = time(NULL); > + nowtime = *localtime(&nowtimer); > + > + runtime = nowtime; > + runtime.tm_sec = s; > + runtime.tm_min = m; > + runtime.tm_hour = h; > + runtime.tm_mday = D; > + runtime.tm_mon = M - 1; > + runtime.tm_year = Y >= 0 ? Y : runtime.tm_year; > + runtime.tm_wday = -1; > + runtime.tm_yday = -1; > + runtime.tm_isdst = -1; > + > + runtimer = mktime(&runtime); > + > + if(runtimer < 0) > + panic("invalid time"); > + > + if(nowtimer > runtimer) > + panic("trying to travel back in time"); > + > + return runtimer; > +} This function seems needlessly complicated. Why not just adapt the stime_arg2() function from touch(1)? I didn't give a full review of this function. If you decide to use your version, let me know and I'll give a more detailed review of this function. > + > + One line is enough. > int > main(int argc, char **argv) > { > @@ -601,9 +731,9 @@ > char *pgm; > > int program = AT; /* our default program */ > - const char *options = "q:f:mvldbVc";/* default options for at */ > + const char *options = "q:f:t:mvlrdbVc";/* default options for at */ > int disp_version = 0; > - time_t timer; > + time_t timer = (time_t) -1; The initialization should be moved below the declaration; see style(9). Also, there is no need to cast -1 here. > > RELINQUISH_PRIVS > > @@ -660,6 +790,9 @@ > break; > > case 'd': > + warnx("-d is deprecated; you should use -r in the future"); I like this. > + /* fall through to 'r' */ > + case 'r': > if (program != AT) > usage(); > > @@ -667,6 +800,12 @@ > options = "V"; > break; > > + case 't': > + program = AT; > + options = "V"; > + timer = ttime(optarg); > + break; > + > case 'l': > if (program != AT) > usage(); > @@ -729,7 +868,13 @@ > break; > > case AT: > - timer = parsetime(argc, argv); > + /* > + * If timer is > -1, then the user gave the time with -t. > + * In that case, it's already been set. If not, set it now. > + */ > + if(timer == (time_t) -1) > + timer = parsetime(argc, argv); Again, -1 doesn't need a cast here. > + Gratuitous vertical space. > if (atverify) > { > struct tm *tm = localtime(&timer); > Index: at/at.man > =================================================================== > RCS file: /home/ncvs/src/usr.bin/at/at.man,v > retrieving revision 1.22 > diff -u -r1.22 at.man > --- at/at.man 2001/11/20 15:43:25 1.22 > +++ at/at.man 2001/12/19 03:30:03 > @@ -115,6 +115,8 @@ > and to run a job at 1am tomorrow, you would do > .Nm at Ar 1am tomorrow . > .Pp > +You may also use the POSIX time format (see -t argument) > +.Pp > For both > .Nm > and > @@ -214,7 +216,7 @@ > .Nm atq . > .It Fl d > Is an alias for > -.Nm atrm . > +.Nm atrm (this option is deprecated; use -r instead) . > .It Fl b > Is an alias for > .Nm batch . > @@ -225,6 +227,12 @@ > shows the time the job will be executed. > .It Fl c > Cat the jobs listed on the command line to standard output. > +.It Fl r > +Remove specified job(s). > +.It Fl t > +Give the job time using the POSIX time format, which is described in the > +touch(1) man page. > +Note that giving a date past 2038 may not work on 32-bit systems. > .El > .Sh FILES > .Bl -tag -width _ATJOB_DIR/_LOCKFILE -compact > @@ -272,3 +280,5 @@ > .An Thomas Koenig Aq ig25@rz.uni-karlsruhe.de . > The time parsing routines are by > .An David Parsons Aq orc@pell.chi.il.us . The period should be changed to a comma. > +and This could be better written, "and latter enhanced by..." > +.An Joe Halpin Aq joe.halpin@attbi.com . Best regards, Mike Barcroft To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-standards" in the body of the message