Skip site navigation (1)Skip section navigation (2)
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>

next in thread | previous in thread | raw e-mail | index | archive | help
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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20011228031537.B99161>