Re: BSD strip(1) error handling

[Tobias Ulmer]

Too bad, I didn't know that obscure rule :( I've checked GNU strip and
it also continues on errors.

Thanks for reviewing this patch!

Re: BSD strip(1) error handling

[Philip Guenther]

On Thu, Mar 1, 2012 at 6:29 PM, Tobias Ulmer  wrote:
...
> strip also tries to loop over multiple files despite previous errors and
> returns an error code in the end. There are gotos sprinkled throughout
> to support this. I don't think that is helpful at all and have changed
> it to immediately terminate.

Umm, no.  To quote POSIX, XCU, Section 1.4, "CONSEQUENCES OF ERRORS":
-
  The following shall apply to each utility, unless otherwise stated:
 7 If the requested action cannot be performed on an operand
representing a file,
   directory, user, process, and so on, the utility shall issue a
diagnostic message to
   standard error and continue processing the next operand in
sequence, but the
   final exit status shall be returned as non-zero.
-

strip's description then says:
-
  CONSEQUENCES OF ERRORS
 Default.
-

So the part of your patch that makes it abort on the first error is
certainly not acceptable.


Philip Guenther

BSD strip(1) error handling

[Tobias Ulmer]

strip's error handling appears to be questionable. The ERROR() define
has a continue statement at the end, yet in use there are statements
behind it. gcc2 does not warn about this.

strip also tries to loop over multiple files despite previous errors and
returns an error code in the end. There are gotos sprinkled throughout
to support this. I don't think that is helpful at all and have changed
it to immediately terminate.

This patch survived a hp300 make build

Index: strip.c
===
RCS file: /home/vcs/cvs/openbsd/src/usr.bin/strip/strip.c,v
retrieving revision 1.29
diff -u -p -r1.29 strip.c
--- strip.c 1 Jun 2010 21:44:39 -   1.29
+++ strip.c 2 Mar 2012 01:52:26 -
@@ -78,7 +78,7 @@ main(int argc, char *argv[])
EXEC *ep;
struct stat sb;
int (*sfcn)(const char *, int, EXEC *, struct stat *, off_t *);
-   int ch, errors;
+   int ch;
char *fn, *ofile = NULL;
off_t newsize;
 
@@ -109,32 +109,24 @@ main(int argc, char *argv[])
 
if (ofile != NULL && argc > 1)
usage();
-   errors = 0;
-#defineERROR(x) errors |= 1; warnx("%s: %s", fn, strerror(x)); 
continue;
while ((fn = *argv++)) {
if (ofile) {
char buf[8192];
-   ssize_t wn;
-   size_t rn;
+   ssize_t wn, rn;
off_t off;
int wfd;
 
-   if ((fd = open(fn, O_RDONLY)) < 0) {
-   ERROR(errno);
-   break;
-   }
-   if ((wfd = open(ofile, O_RDWR|O_CREAT)) < 0) {
-   ERROR(errno);
-   break;
-   }
+   if ((fd = open(fn, O_RDONLY)) < 0)
+   err(1, "open: %s", fn);
+
+   if ((wfd = open(ofile, O_RDWR|O_CREAT)) < 0)
+   err(1, "open: %s", ofile);
do {
rn = read(fd, buf, sizeof buf);
-   if (rn == (ssize_t)-1) {
-   int save_errno = errno;
-
-   unlink(ofile);
-   ERROR(save_errno);
-   exit(errors);
+   if (rn == -1) {
+   warn("read: %s", fn);
+   (void) unlink(ofile);
+   exit(1);
}
if (rn == 0)
break;
@@ -142,41 +134,35 @@ main(int argc, char *argv[])
off = 0;
while (rn - off > 0) {
wn = write(wfd, buf + off, rn - off);
-   if (wn == (ssize_t)-1) {
-   int save_errno = errno;
-
-   unlink(ofile);
-   ERROR(save_errno);
-   exit(errors);
+   if (wn == -1) {
+   warn("write: %s", ofile);
+   (void) unlink(ofile);
+   exit(1);
}
off += wn;
}
} while (rn > 0);
 
fn = ofile;
-   close(fd);
+   (void) close(fd);
fd = wfd;
} else if ((fd = open(fn, O_RDWR)) < 0) {
-   ERROR(errno);
-   }
-   if (fstat(fd, &sb)) {
-   (void)close(fd);
-   ERROR(errno);
-   }
-   if (sb.st_size < sizeof(EXEC)) {
-   (void)close(fd);
-   ERROR(EFTYPE);
-   }
-   if ((ep = (EXEC *)mmap(NULL, sb.st_size, PROT_READ|PROT_WRITE,
-   MAP_SHARED, fd, (off_t)0)) == MAP_FAILED) {
-   (void)close(fd);
-   ERROR(errno);
+   err(1, "open: %s", fn);
}
-   if (BAD_OBJECT(*ep)) {
-   munmap((caddr_t)ep, sb.st_size);
-   (void)close(fd);
-   ERROR(EFTYPE);
+
+   if (fstat(fd, &sb))
+   err(1, "fstat: %s", fn);
+
+   if (sb.st_size < sizeof(EXEC))
+