Re: Improve install -s

kettenis' version made -s failures fatal.  That does not match
tradition.  Originally the file was put into place and then strip was
run to "attempt" to make symbol table reduction changes.  So if strip
fails, shrug, the complete file is ready for use and install would exit
0.

Since the new code runs strip -o, we cannot be exactly certain of the outcome
and have to do it carefully and correctly ourselves.

Since it is just a request, -s is often used accidentally on all sorts
of non-binary files.

After a whole day of code iterations, I've have arrived at this version.

If strip fails, some cleanup happens and the body of the install()
function is-rerun without the strip request which will cause it to do
the regular copy operations.

Index: usr.bin/xinstall/install.1
===================================================================
RCS file: /cvs/src/usr.bin/xinstall/install.1,v
diff -u -p -u -r1.31 install.1
--- usr.bin/xinstall/install.1	8 Feb 2019 12:53:44 -0000	1.31
+++ usr.bin/xinstall/install.1	22 Aug 2026 19:15:56 -0000
@@ -147,11 +147,13 @@ in the destination directory, then atomi
 This avoids both race conditions and the destruction of existing
 files in case of write failures.
 .It Fl s
-.Nm
-exec's the command
+Attempt to strip the file of unneccessary sections and symbols.
+This executes the command
 .Pa /usr/bin/strip
-to strip binaries so that install can be portable over a large
-number of systems and binary types.
+using the
+.Fl o
+option to produce the output file, and if that fails copies the
+file in the regular way.
 If the environment variable
 .Ev STRIP
 is set, it is used instead.
@@ -175,7 +177,9 @@ creates an empty file.
 .It Ev STRIP
 For an alternate
 .Xr strip 1
-program to run.
+program to run, which must support the
+.Fl o
+option.
 Default is
 .Pa /usr/bin/strip .
 .El
Index: usr.bin/xinstall/xinstall.c
===================================================================
RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
diff -u -p -u -r1.78 xinstall.c
--- usr.bin/xinstall/xinstall.c	17 Oct 2024 15:38:38 -0000	1.78
+++ usr.bin/xinstall/xinstall.c	22 Aug 2026 22:41:39 -0000
@@ -72,7 +72,7 @@ void	copy(int, char *, int, char *, off_
 int	compare(int, const char *, off_t, int, const char *, off_t);
 void	install(char *, char *, u_long, u_int);
 void	install_dir(char *, int);
-void	strip(char *);
+int	strip(char *, char *);
 void	usage(void);
 int	create_tempfile(char *, char *, size_t);
 int	file_write(int, char *, size_t, int *, int *, int);
@@ -220,7 +220,8 @@ install(char *from_name, char *to_name, 
 {
 	struct stat from_sb, to_sb;
 	struct timespec ts[2];
-	int devnull, from_fd, to_fd, serrno, files_match = 0;
+	int devnull, from_fd = -1, to_fd = -1, serrno, files_match = 0;
+	int l_dostrip = dostrip, l_docompare = docompare;
 	char *p;
 	char *target_name = tempfile;
 
@@ -247,44 +248,61 @@ install(char *from_name, char *to_name, 
 
 	if (stat(to_name, &to_sb) == 0) {
 		/* Only compare against regular files. */
-		if (docompare && !S_ISREG(to_sb.st_mode)) {
-			docompare = 0;
+		if (l_docompare && !S_ISREG(to_sb.st_mode)) {
+			l_docompare = 0;
 			warnc(EFTYPE, "%s", to_name);
 		}
-	} else if (docompare) {
+	} else if (l_docompare) {
 		/* File does not exist so silently ignore compare flag. */
-		docompare = 0;
+		l_docompare = 0;
 	}
 
-	if (!devnull) {
+again:
+	if (!devnull && !l_dostrip) {
 		if ((from_fd = open(from_name, O_RDONLY)) == -1)
 			err(1, "%s", from_name);
 	}
 
 	to_fd = create_tempfile(to_name, tempfile, sizeof(tempfile));
 	if (to_fd < 0)
-		err(1, "%s", tempfile);
+		err(1, "mkstemp %s", tempfile);
 
-	if (!devnull)
+	if (!devnull && !l_dostrip)
 		copy(from_fd, from_name, to_fd, tempfile, from_sb.st_size,
 		    ((off_t)from_sb.st_blocks * S_BLKSIZE < from_sb.st_size));
 
-	if (dostrip) {
-		strip(tempfile);
-
-		/*
-		 * Re-open our fd on the target, in case we used a strip
-		 *  that does not work in-place -- like gnu binutils strip.
-		 */
-		close(to_fd);
-		if ((to_fd = open(tempfile, O_RDONLY)) == -1)
-			err(1, "stripping %s", to_name);
+	if (l_dostrip) {
+		if (strip(from_name, tempfile) == -1) {
+			/*
+			 * Don't know why it failed. Input file does not
+			 * exist?  Not a strippable binary?  Out of disk
+			 * space?  An error message has been issued, but
+			 * -s is a request to attempt stripping of the
+			 * resulting file. Try again without the strip attempt.
+			 */
+			if (from_fd != -1)
+				close(from_fd);
+			close(to_fd);
+			(void)unlink(tempfile);
+			l_dostrip = 0;
+			goto again;
+		} else {
+			/*
+			 * strip(1) may have created a new file after
+			 * unlink(2) or used rename(2) of a different
+			 * file, so we must re-open.  This is an ugly
+			 * race against a long mktemp filename.
+			 */
+			close(to_fd);
+			if ((to_fd = open(tempfile, O_RDONLY)) == -1)
+				err(1, "%s", to_name);
+		}
 	}
 
 	/*
 	 * Compare the (possibly stripped) temp file to the target.
 	 */
-	if (docompare) {
+	if (l_docompare) {
 		int temp_fd = to_fd;
 		struct stat temp_sb;
 
@@ -531,8 +549,8 @@ compare(int from_fd, const char *from_na
  * strip --
  *	use strip(1) to strip the target file
  */
-void
-strip(char *to_name)
+int
+strip(char *from_name, char *to_name)
 {
 	int serrno, status;
 	char * volatile path_strip;
@@ -547,7 +565,8 @@ strip(char *to_name)
 		(void)unlink(to_name);
 		errc(1, serrno, "forks");
 	case 0:
-		execl(path_strip, "strip", "--", to_name, (char *)NULL);
+		execl(path_strip, "strip", "-o", to_name, "--", from_name,
+		    (char *)NULL);
 		warn("%s", path_strip);
 		_exit(1);
 	default:
@@ -555,9 +574,10 @@ strip(char *to_name)
 			if (errno != EINTR)
 				break;
 		}
-		if (!WIFEXITED(status))
-			(void)unlink(to_name);
+		if (!WIFEXITED(status) || WEXITSTATUS(status))
+			return -1;
 	}
+	return 0;
 }
 
 /*