Date: Fri, 28 Dec 2001 03:15:37 -0500 From: Mike Barcroft <mike@FreeBSD.ORG> To: Joe Halpin <joe.halpin@attbi.com> Cc: "standards@FreeBSD.ORG" <standards@FreeBSD.ORG> Subject: Re: at utility changes Message-ID: <20011228031537.B99161@espresso.q9media.com> In-Reply-To: <3C200BBA.9D26ED93@attbi.com>; from joe.halpin@attbi.com on Tue, Dec 18, 2001 at 09:38:34PM -0600 References: <3C200BBA.9D26ED93@attbi.com>
index | next in thread | previous in thread | raw e-mail
Joe Halpin <joe.halpin@attbi.com> 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
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20011228031537.B99161>