Subject: RFC: lpr message output and exit status cleanup
To: None <tech-userlevel@netbsd.org>
From: Brian Ginsbach <ginsbach@cray.com>
List: tech-userlevel
Date: 03/25/2003 21:38:20
I'm wondering what people think of the following changes
to lpr et al.  These changes were inspired by several things.

 - lpr isn't consistent using stdout and stderr for messages
   Most of the printf()s should be changed to warn()/warnx() calls.
   (Similar to what was done in the OpenBSD version.)

 - lpr doesn't exit non-zero if it can't spool one file out of many 
   (This was caught by a POSIX test using the lp wrapper to lpr.)

 - lpr/common_source/fatal.c:fatal() still relies on lp{c,d,q,r,rm}
   to set name = argv[0], when it should really use getprogname()
 
 - lpr doesn't really need fatal2() as it is identical to errx()
   All fatal2() calls should be replaced with errx() calls.  Then
   fatal2() can be removed.  (Simillar to what was done in the
   FreeBSD and OpenBSD versions.)

If others agree that these are "good" changes then I'll submit
a PR (unless someone wants to commit directly.)

Note the diffs below also include changes for PR bin/20890 and
standards/17916.  Hint, hint... :-)

Index: common_source/common_vars.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/common_source/common_vars.c,v
retrieving revision 1.1
diff -u -r1.1 common_vars.c
--- common_source/common_vars.c	1999/12/05 22:10:57	1.1
+++ common_source/common_vars.c	2003/03/26 03:09:23
@@ -47,7 +47,6 @@
 
 #include "pathnames.h"
 
-char	*name;			/* program name */
 char	*printer;		/* printer name */
 char	host[MAXHOSTNAMELEN+1];	/* host machine name */
 char	*from = host;		/* client's machine name */
Index: common_source/fatal.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/common_source/fatal.c,v
retrieving revision 1.3
diff -u -r1.3 fatal.c
--- common_source/fatal.c	2002/07/14 15:27:58	1.3
+++ common_source/fatal.c	2003/03/26 03:09:23
@@ -59,7 +59,7 @@
 	va_start(ap, msg);
 	if (from != host)
 		(void)printf("%s: ", host);
-	(void)printf("%s: ", name);
+	(void)printf("%s: ", getprogname());
 	if (printer)
 		(void)printf("%s: ", printer);
 	(void)vprintf(msg, ap);
Index: common_source/lp.h
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/common_source/lp.h,v
retrieving revision 1.16
diff -u -r1.16 lp.h
--- common_source/lp.h	2002/07/14 15:27:58	1.16
+++ common_source/lp.h	2003/03/26 03:09:23
@@ -83,7 +83,6 @@
 
 extern char	line[BUFSIZ];
 extern char	*bp;		/* pointer into printcap buffer */
-extern char	*name;		/* program name */
 extern char	*printer;	/* printer name */
 				/* host machine name */
 extern char	host[MAXHOSTNAMELEN + 1];
Index: lp/lp
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lp/lp,v
retrieving revision 1.4
diff -u -r1.4 lp
--- lp/lp	1998/07/07 17:05:28	1.4
+++ lp/lp	2003/03/26 03:09:23
@@ -42,6 +42,7 @@
 
 ncopies=""
 symlink="-s"
+reqid="-R"
 
 # Posix says LPDEST gets precedence over PRINTER
 dest=${LPDEST:-${PRINTER:-lp}}
@@ -57,7 +58,7 @@
 	c)			# copy files before printing
 		symlink="";;
 	s)			# silent
-		: ;;
+		reqid="";;
 	d)			# destination
 		dest="${OPTARG}";;
 	n)			# number of copies
@@ -71,4 +72,4 @@
 
 shift $(($OPTIND - 1))
 
-exec /usr/bin/lpr "-P${dest}" ${symlink} ${ncopies} "$@"
+exec /usr/bin/lpr "-P${dest}" ${reqid} ${symlink} ${ncopies} "$@"
Index: lpc/lpc.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lpc/lpc.c,v
retrieving revision 1.14
diff -u -r1.14 lpc.c
--- lpc/lpc.c	2002/07/20 08:40:18	1.14
+++ lpc/lpc.c	2003/03/26 03:09:23
@@ -97,7 +97,6 @@
 	euid = geteuid();
 	uid = getuid();
 	seteuid(uid);
-	name = argv[0];
 	openlog("lpd", 0, LOG_LPR);
 
 	if (--argc > 0) {
Index: lpd/lpd.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lpd/lpd.c,v
retrieving revision 1.44
diff -u -r1.44 lpd.c
--- lpd/lpd.c	2002/10/26 01:46:31	1.44
+++ lpd/lpd.c	2003/03/26 03:09:24
@@ -161,7 +161,6 @@
 	uid = getuid();
 	gethostname(host, sizeof(host));
 	host[sizeof(host) - 1] = '\0';
-	name = argv[0];
 
 	errs = 0;
 	while ((i = getopt(argc, argv, "b:dln:srw:W")) != -1)
Index: lpq/lpq.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lpq/lpq.c,v
retrieving revision 1.12
diff -u -r1.12 lpq.c
--- lpq/lpq.c	2002/07/14 15:28:00	1.12
+++ lpq/lpq.c	2003/03/26 03:09:24
@@ -88,7 +88,6 @@
 	euid = geteuid();
 	uid = getuid();
 	seteuid(uid);
-	name = *argv;
 	if (gethostname(host, sizeof(host)))
 		err(1, "lpq: gethostname");
 	host[sizeof(host) - 1] = '\0';
Index: lpr/lpr.1
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lpr/lpr.1,v
retrieving revision 1.12
diff -u -r1.12 lpr.1
--- lpr/lpr.1	2003/02/25 10:36:12	1.12
+++ lpr/lpr.1	2003/03/26 03:09:24
@@ -41,7 +41,7 @@
 .Nd off line print
 .Sh SYNOPSIS
 .Nm
-.Op Fl cdfghlmnprstv
+.Op Fl cdfghlmnqprRstv
 .Bk -words
 .Op Fl P Ar printer
 .Ek
@@ -132,6 +132,8 @@
 Suppress the printing of the burst page.
 .It Fl m
 Send mail upon completion.
+.It Fl q
+Queue the print job but do not start the spooling daemon.
 .It Fl r
 Remove the file upon completion of spooling or upon completion of
 printing (with the
@@ -146,6 +148,20 @@
 to link data files rather than trying to copy them so large files can be
 printed.  This means the files should
 not be modified or removed until they have been printed.
+.El
+.Pp
+Normally
+.Nm
+works silently except for diaganostic messages.  The following 
+option changes this behavior.
+.Bl -tag -width indent
+.It Fl R
+Writes a message to standard output containing the unique number which
+is used to identify this job.  This number can be used to cancel (see
+.Xr lprm 1 )
+or find the status (see
+.Xr lpq 1 )
+of the job.
 .El
 .Pp
 The remaining options apply to copies, the page display, and headers:
Index: lpr/lpr.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lpr/lpr.c,v
retrieving revision 1.23
diff -u -r1.23 lpr.c
--- lpr/lpr.c	2002/07/14 15:28:00	1.23
+++ lpr/lpr.c	2003/03/26 03:09:24
@@ -94,7 +94,9 @@
 static int	 ncopies = 1;	/* # of copies to make */
 static const char *person;	/* user name */
 static int	 qflag;		/* q job, but don't exec daemon */
+static int	 reqid;		/* request id */
 static int	 rflag;		/* remove files upon completion */	
+static int	 Rflag;		/* print request id - like POSIX lp */
 static int	 sflag;		/* symbolic link flag */
 static int	 tfd;		/* control file descriptor */
 static char	*tfname;	/* tmp copy of cf before linking */
@@ -108,8 +110,6 @@
 static void	 chkprinter(char *);
 static void	 cleanup(int);
 static void	 copy(int, char []);
-static void	 fatal2(const char *, ...)
-	__attribute__((__format__(__printf__, 1, 2)));
 static char	*itoa(int);
 static char	*linked(char *);
 static char	*lmktemp(char *, int, int);
@@ -130,6 +130,7 @@
 	char buf[MAXPATHLEN];
 	int i, f, errs, c;
 	struct stat stb;
+	int nrequests = 0;
 
 	euid = geteuid();
 	uid = getuid();
@@ -144,14 +145,13 @@
 	if (signal(SIGTERM, SIG_IGN) != SIG_IGN)
 		signal(SIGTERM, cleanup);
 
-	name = argv[0];
 	gethostname(host, sizeof (host));
 	host[sizeof(host) - 1] = '\0';
 	openlog("lpd", 0, LOG_LPR);
 
 	errs = 0;
 	while ((c = getopt(argc, argv,
-	    ":#:1:2:3:4:C:J:P:T:U:cdfghi:lmnprstvw:")) != -1) {
+	    ":#:1:2:3:4:C:J:P:RT:U:cdfghi:lmnqprstvw:")) != -1) {
 		switch (c) {
 
 		case '#':		/* n copies */
@@ -183,6 +183,10 @@
 			printer = optarg;
 			break;
 
+		case 'R':		/* print request id */
+			Rflag++;
+			break;
+			
 		case 'T':		/* pr's title line */
 			title = optarg;
 			break;
@@ -258,9 +262,9 @@
 		printer = DEFLP;
 	chkprinter(printer);
 	if (SC && ncopies > 1)
-		fatal2("multiple copies are not allowed");
+		errx(1, "multiple copies are not allowed");
 	if (MC > 0 && ncopies > MC)
-		fatal2("only %ld copies are allowed", MC);
+		errx(1, "only %ld copies are allowed", MC);
 	/*
 	 * Get the identity of the person doing the lpr using the same
 	 * algorithm as lprm. 
@@ -268,7 +272,7 @@
 	userid = getuid();
 	if (userid != DU || person == 0) {
 		if ((pw = getpwuid(userid)) == NULL)
-			fatal2("Who are you?");
+			errx(1, "Who are you?");
 		person = pw->pw_name;
 	}
 	/*
@@ -276,7 +280,7 @@
 	 */
 	if (RG != NULL && userid != DU) {
 		if ((gptr = getgrnam(RG)) == NULL)
-			fatal2("Restricted group specified incorrectly");
+			errx(1, "Restricted group specified incorrectly");
 		if (gptr->gr_gid != getgid()) {
 			while (*gptr->gr_mem != NULL) {
 				if ((strcmp(person, *gptr->gr_mem)) == 0)
@@ -284,7 +288,7 @@
 				gptr->gr_mem++;
 			}
 			if (*gptr->gr_mem == NULL)
-				fatal2("Not a member of the restricted group");
+				errx(1, "Not a member of the restricted group");
 		}
 	}
 	/*
@@ -292,7 +296,7 @@
 	 */
 	(void)snprintf(buf, sizeof buf, "%s/%s", SD, LO);
 	if (userid && stat(buf, &stb) == 0 && (stb.st_mode & S_IXGRP))
-		fatal2("Printer queue is disabled");
+		errx(1, "Printer queue is disabled");
 	/*
 	 * Initialize the control file.
 	 */
@@ -329,17 +333,21 @@
 	/*
 	 * Read the files and spool them.
 	 */
-	if (argc == 0)
+	if (argc == 0) {
+		nrequests++;
 		copy(0, " ");
+	}
 	else while (argc--) {
+		nrequests++;
 		if (argv[0][0] == '-' && argv[0][1] == '\0') {
 			/* use stdin */
 			copy(0, " ");
 			argv++;
 			continue;
 		}
-		if ((f = test(arg = *argv++)) < 0)
+		if ((f = test(arg = *argv++)) < 0) {
 			continue;	/* file unreasonable */
+		}
 
 		if (sflag && (cp = linked(arg)) != NULL) {
 			(void)snprintf(buf, sizeof buf,
@@ -358,17 +366,18 @@
 			continue;
 		}
 		if (sflag)
-			printf("%s: %s: not linked, copying instead\n", name, arg);
+			warnx("%s: not linked, copying instead", arg);
 		seteuid(uid);
 		if ((i = open(arg, O_RDONLY)) < 0) {
 			seteuid(uid);
-			printf("%s: cannot open %s\n", name, arg);
+			warn("cannot open %s", arg);
 			continue;
 		} else {
 			copy(i, arg);
 			(void)close(i);
-			if (f && unlink(arg) < 0)
-				printf("%s: %s: not removed\n", name, arg);
+			if (f && unlink(arg) < 0) {
+				warn("%s: not removed", arg);
+			}
 		}
 		seteuid(uid);
 	}
@@ -386,24 +395,27 @@
 			if (read(tfd, &c, 1) == 1 &&
 			    lseek(tfd, (off_t)0, 0) == 0 &&
 			    write(tfd, &c, 1) != 1) {
-				printf("%s: cannot touch %s\n", name, tfname);
+				warn("cannot touch %s", tfname);
 				tfname[inchar]++;
 				cleanup(0);
 			}
 			(void)close(tfd);
 		}
 		if (link(tfname, cfname) < 0) {
-			printf("%s: cannot rename %s\n", name, cfname);
+			warn("cannot rename %s", cfname);
 			tfname[inchar]++;
 			cleanup(0);
 		}
 		unlink(tfname);
 		seteuid(uid);
+		if (Rflag)
+			printf("Request id is %d\n", reqid);
 		if (qflag)		/* just q things up */
-			exit(0);
+			exit(nact != nrequests);
 		if (!startdaemon(printer))
 			printf("jobs queued, but cannot start daemon.\n");
-		exit(0);
+		
+		exit(nact != nrequests);
 	}
 	cleanup(0);
 #ifdef __GNUC__
@@ -431,7 +443,7 @@
 	nr = nc = 0;
 	while ((i = read(f, buf, sizeof buf)) > 0) {
 		if (write(fd, buf, i) != i) {
-			printf("%s: %s: temp file write error\n", name, n);
+			warn("%s", n);
 			break;
 		}
 		nc += i;
@@ -439,15 +451,15 @@
 			nc -= sizeof buf;
 			nr++;
 			if (MX > 0 && nr > MX) {
-				printf("%s: %s: copy file is too large "
-				    "(check :mx:?)\n", name, n);
+				warnx("%s: copy file is too large "
+				    "(check :mx:?)", n);
 				break;
 			}
 		}
 	}
 	(void)close(fd);
 	if (nc==0 && nr==0) 
-		printf("%s: %s: empty input file\n", name, f ? n : "stdin");
+		warnx("%s: empty input file", f ? n : "stdin");
 	else
 		nact++;
 }
@@ -527,17 +539,17 @@
 	f = open(n, O_WRONLY | O_EXCL | O_CREAT, FILMOD);
 	(void)umask(oldumask);
 	if (f < 0) {
-		printf("%s: cannot create %s\n", name, n);
+		warn("cannot create %s", n);
 		cleanup(0);
 	}
 	if (fchown(f, userid, -1) < 0) {
-		printf("%s: cannot chown %s\n", name, n);
+		warn("cannot chown %s", n);
 		cleanup(0);	/* cleanup does exit */
 	}
 	seteuid(uid);
 	if (++n[inchar] > 'z') {
 		if (++n[inchar-2] == 't') {
-			printf("too many files - break up the job\n");
+			warnx("too many files - break up the job");
 			cleanup(0);
 		}
 		n[inchar] = 'A';
@@ -592,29 +604,29 @@
 
 	seteuid(uid);
 	if (access(file, 4) < 0) {
-		printf("%s: cannot access %s\n", name, file);
+		warn("cannot access %s", file);
 		goto bad;
 	}
 	if (stat(file, &statb) < 0) {
-		printf("%s: cannot stat %s\n", name, file);
+		warn("cannot stat %s", file);
 		goto bad;
 	}
 	if (S_ISDIR(statb.st_mode)) {
-		printf("%s: %s is a directory\n", name, file);
+		warnx("%s is a directory", file);
 		goto bad;
 	}
 	if (statb.st_size == 0) {
-		printf("%s: %s is an empty file\n", name, file);
+		warnx("%s is an empty file", file);
 		goto bad;
  	}
 	if ((fd = open(file, O_RDONLY)) < 0) {
-		printf("%s: cannot open %s\n", name, file);
+		warn("cannot open %s", file);
 		goto bad;
 	}
 	if (read(fd, &execb, sizeof(execb)) == sizeof(execb) &&
 	    !N_BADMAG(execb)) {
-			printf("%s: %s is an executable program and is unprintable",
-				name, file);
+			warnx("%s is an executable program and is unprintable",
+				file);
 			(void)close(fd);
 			goto bad;
 	}
@@ -634,7 +646,7 @@
 			if (fd == 0)
 				return(1);
 		}
-		printf("%s: %s: is not removable by you\n", name, file);
+		warnx("%s: is not removable by you", file);
 	}
 	return(0);
 bad:
@@ -667,9 +679,9 @@
 	int status;
 
 	if ((status = cgetent(&bp, printcapdb, s)) == -2)
-		fatal2("cannot open printer description file");
+		errx(1, "cannot open printer description file");
 	else if (status == -1)
-		fatal2("%s: unknown printer", s);
+		errx(1, "%s: unknown printer", s);
 	if (cgetstr(bp, "sd", &SD) == -1)
 		SD = _PATH_DEFSPOOL;
 	if (cgetstr(bp, "lo", &LO) == -1)
@@ -697,12 +709,10 @@
 	(void)snprintf(buf, BUFSIZ, "%s/.seq", SD);
 	seteuid(euid);
 	if ((fd = open(buf, O_RDWR|O_CREAT, 0661)) < 0) {
-		printf("%s: cannot create %s\n", name, buf);
-		exit(1);
+		err(1, "cannot create %s", buf);
 	}
 	if (flock(fd, LOCK_EX)) {
-		printf("%s: cannot lock %s\n", name, buf);
-		exit(1);
+		err(1, "cannot lock %s", buf);
 	}
 	seteuid(uid);
 	n = 0;
@@ -713,6 +723,7 @@
 			n = n * 10 + (*cp++ - '0');
 		}
 	}
+	reqid = n;
 	len = strlen(SD) + strlen(host) + 8;
 	tfname = lmktemp("tf", n, len);
 	cfname = lmktemp("cf", n, len);
@@ -734,26 +745,11 @@
 	char *s;
 
 	if ((s = malloc(len)) == NULL)
-		fatal2("out of memory");
+		errx(1, "out of memory");
 	(void)snprintf(s, len, "%s/%sA%03d%s", SD, id, num, host);
 	return(s);
 }
 
-#include <stdarg.h>
-
-static void
-fatal2(const char *msg, ...)
-{
-	va_list ap;
-
-	va_start(ap, msg);
-	printf("%s: ", name);
-	vprintf(msg, ap);
-	putchar('\n');
-	va_end(ap);
-	exit(1);
-}
-
 static void
 usage(void)
 {
@@ -761,6 +757,6 @@
 	fprintf(stderr, "%s\n%s\n",
 	    "usage: lpr [-Pprinter] [-#num] [-C class] [-J job] [-T title] "
 	    "[-U user]",
-	    "[-i[numcols]] [-1234 font] [-wnum] [-cdfghlmnprstv] [name ...]");
+	    "[-i[numcols]] [-1234 font] [-wnum] [-cdfghlmnqprstv] [name ...]");
 	exit(1);
 }
Index: lprm/lprm.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/lpr/lprm/lprm.c,v
retrieving revision 1.13
diff -u -r1.13 lprm.c
--- lprm/lprm.c	2002/07/14 15:28:01	1.13
+++ lprm/lprm.c	2003/03/26 03:09:24
@@ -95,7 +95,6 @@
 	uid = getuid();
 	euid = geteuid();
 	seteuid(uid);	/* be safe */
-	name = argv[0];
 	gethostname(host, sizeof(host));
 	host[sizeof(host) - 1] = '\0';
 	openlog("lpd", 0, LOG_LPR);