Date: Sat, 29 Dec 2001 15:33:28 -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: <20011229153328.D99161@espresso.q9media.com> In-Reply-To: <3C2DF35D.1F54BBC3@attbi.com>; from joe.halpin@attbi.com on Sat, Dec 29, 2001 at 10:46:21AM -0600 References: <3C200BBA.9D26ED93@attbi.com> <20011228031537.B99161@espresso.q9media.com> <3C2DF35D.1F54BBC3@attbi.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Joe Halpin <joe.halpin@attbi.com> writes:
> Here are a new set of changes. I think I got the style more or less
> right this time, and cloned the stime_arg1() function from touch.
I like this version much better. It only needs a few changes.
> Index: at/at.c
> ===================================================================
> RCS file: /home/ncvs/src/usr.bin/at/at.c,v
> retrieving revision 1.18.2.1
> diff -u -r1.18.2.1 at.c
> --- at/at.c 2001/08/02 00:55:58 1.18.2.1
> +++ at/at.c 2001/12/29 16:41:19
> @@ -121,6 +121,7 @@
> static char *cwdname(void);
> static void writefile(time_t runtimer, char queue);
> static void list_jobs(void);
> +static time_t ttime(const char *arg);
>
> /* Signal catching functions */
>
> @@ -588,6 +589,80 @@
> }
> } /* delete_jobs */
>
> +#define ATOI2(ar) ((ar)[0] - '0') * 10 + ((ar)[1] - '0'); (ar) += 2;
> +
> +static time_t
> +ttime(const char *arg)
> +{
> + /*
> + * This is pretty much a copy of stime_arg1() from touch.c. I changed
This needs a second space after the period.
> + * the return value and the argument list because it's more convenient
> + * (IMO) to do everything in one place. - Joe Halpin
> + */
> + struct timeval tv[2];
> + time_t now;
> + struct tm *t;
> + int yearset;
> + char *p;
> +
> + if (gettimeofday(&tv[0], NULL))
> + panic("Cannot get current time");
> +
> + /* Start with the current time. */
> + now = tv[0].tv_sec;
> + if ((t = localtime(&now)) == NULL)
> + panic("localtime");
> + /* [[CC]YY]MMDDhhmm[.SS] */
> + if ((p = strchr(arg, '.')) == NULL)
> + t->tm_sec = 0; /* Seconds defaults to 0. */
> + else {
> + if (strlen(p + 1) != 2)
> + goto terr;
> + *p++ = '\0';
> + t->tm_sec = ATOI2(p);
> + }
> +
> + yearset = 0;
> + switch(strlen(arg)) {
> + case 12: /* CCYYMMDDhhmm */
> + t->tm_year = ATOI2(arg);
> + t->tm_year *= 100;
> + yearset = 1;
> + /* FALLTHROUGH */
> + case 10: /* YYMMDDhhmm */
> + if (yearset) {
> + yearset = ATOI2(arg);
> + t->tm_year += yearset;
> + } else {
> + yearset = ATOI2(arg);
> + if (yearset < 69)
> + t->tm_year = yearset + 2000;
> + else
> + t->tm_year = yearset + 1900;
> + }
The else section can be reduced to something like:
%%%
yearset = ATOI2(arg);
t->tm_year = yearset + 2000;
%%%
...since it's impossible to schedule events to take place in the past.
> + t->tm_year -= 1900; /* Convert to UNIX time. */
> + /* FALLTHROUGH */
> + case 8: /* MMDDhhmm */
> + t->tm_mon = ATOI2(arg);
> + --t->tm_mon; /* Convert from 01-12 to 00-11 */
> + t->tm_mday = ATOI2(arg);
> + t->tm_hour = ATOI2(arg);
> + t->tm_min = ATOI2(arg);
> + break;
> + default:
> + goto terr;
> + }
> +
> + t->tm_isdst = -1; /* Figure out DST. */
> + tv[0].tv_sec = tv[1].tv_sec = mktime(t);
> + if (tv[0].tv_sec != -1)
> + return tv[0].tv_sec;
> + else
> + terr:
> + panic("out of range or illegal time specification: [[CC]YY]MMDDhhmm[.SS]");
> + return -1; /* get rid of "control reaches end of non-void function" msg */
The panic and return line should probably be indented more since they
apply to the else case in normal operation. Return values should be
wrapped in parens.
> +}
> +
> int
> main(int argc, char **argv)
> {
> @@ -598,10 +673,12 @@
>
> enum { ATQ, ATRM, AT, BATCH, CAT }; /* what program we want to run */
> int program = AT; /* our default program */
> - char *options = "q:f:mvldbVc"; /* default options for at */
> + char *options = "q:f:t:rmvldbVc"; /* default options for at */
> int disp_version = 0;
> time_t timer;
>
> + timer = -1;
Much better.
> +
Gratuitous vertical space.
> RELINQUISH_PRIVS
>
> /* Eat any leading paths
> @@ -655,6 +732,9 @@
> break;
>
> case 'd':
> + warnx("-d is deprecated; you should use -r in the future");
> + /* fall through to 'r' */
> + case 'r':
> if (program != AT)
> usage();
>
> @@ -662,6 +742,14 @@
> options = "V";
> break;
>
> + case 't':
> + if (program != AT)
> + usage();
> +
This vertical space isn't needed either.
> + options = "V";
> + timer = ttime(optarg);
> + break;
> +
> case 'l':
> if (program != AT)
> usage();
> @@ -696,7 +784,7 @@
>
> if (disp_version)
> fprintf(stderr, "at version " VERSION "\n"
> - "Bug reports to: ig25@rz.uni-karlsruhe.de (Thomas Koenig)\n");
> + "Bug reports to: ig25@rz.uni-karlsruhe.de (Thomas Koenig)\n");
I wonder if the author accepts bug reports on customized versions of
his software.
>
> /* select our program
> */
> @@ -723,7 +811,14 @@
> break;
>
> case AT:
> - timer = parsetime(argc, argv);
> +
Gratuitous vertical space.
> + /*
> + * 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.
> + */
Comments should go to 80 columns, then wrap. Also, this requires a
second space after the period.
> + if (timer == -1)
> + timer = parsetime(argc, argv);
> +
> if (atverify)
> {
> struct tm *tm = localtime(&timer);
usage() needs to be updated to reflect the new options. It's hidden
away in panic.c.
> Index: at/at.man
> ===================================================================
> RCS file: /home/ncvs/src/usr.bin/at/at.man,v
> retrieving revision 1.13.2.7
> diff -u -r1.13.2.7 at.man
> --- at/at.man 2001/12/14 15:53:29 1.13.2.7
> +++ at/at.man 2001/12/29 16:41:19
The SYNOPSIS sections needs to be updated to reflect the new options.
> @@ -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)
Periods complete sentences.
> +.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) .
Please add a new line between `atrm' and `(this', otherwise the whole
section is sent to the macro.
> .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
`Give' should be changed to `Specify'.
> +touch(1) man page.
This should be changed to:
%%%
.Xr touch 1
manual.
%%%
(I prefer manual over man page, but that's an optional change.)
> +Note that giving a date past 2038 may not work on 32-bit systems.
This should be under the BUGS section. It also applies to the Alpha
architecture, so it's probably wise to replace `32-bit' with `some'.
> .El
> .Sh FILES
> .Bl -tag -width _ATJOB_DIR/_LOCKFILE -compact
> @@ -271,4 +279,6 @@
> At was mostly written by
> .An Thomas Koenig Aq ig25@rz.uni-karlsruhe.de .
> The time parsing routines are by
> -.An David Parsons Aq orc@pell.chi.il.us .
> +.An David Parsons Aq orc@pell.chi.il.us ,
> +with minor enhancement by
This should say either "with a minor enhancement by" or
"with minor enhancements by". I prefer the latter.
> +.An Joe Halpin Aq joe.halpin@attbi.com .
A `STANDARDS' section should be added under `SEE ALSO':
%%%
.Sh STANDARDS
The
.Nm
utility conforms with
.St -p1003.1-2001 .
%%%
(Would you please verify this? I only glanced at the POSIX.1-2001
requirements.)
Once you get those problems fixed I should be able to test and commit
this for you.
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?20011229153328.D99161>