From: kn <kn@openbsd.org>
Date: Fri, 9 Sep 2022 15:53:16 +0000 (+0000)
Subject: Reflect script failure in exit code
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=20f1db5491bc29cb04de8e4f6518c684ace7660e;p=openbsd

Reflect script failure in exit code

installboot(8) runs newfs(8) and fsck(8) via system(3) but only checks
failures of the function itself, always returning zero no matter what the
programs/shell returned.

This is bad for regress tests relying on correct return codes.

create_filesystem() itself must not exit as write_filesystem() calls it and
cleans up temporary files upon failure.

Make it return -1 if the script returned non-zero so write_filesystem()
handles it as error, cleans up and makes installboot exit 1.

Stop ignoring create_filesystem()'s return code in md_prepareboot() and
exit the same way.

Here's the change in behaviour on arm64 (newfs fails because of the
vnd/disklabel race, see "Race in disk_attach_callback?" on tech@):

	# installboot -vp vnd0 ; echo $?
	newfsing 6694ae5b0d7596ed.i
	newfs_msdos: /dev/r6694ae5b0d7596ed.i: No such file or directory
	0
	# ./obj/installboot -vp vnd0 ; echo $?
	newfsing 6694ae5b0d7596ed.i
	newfs_msdos: /dev/r6694ae5b0d7596ed.i: No such file or directory
	1

Tested on amd64 arm64 macppc octeon powerpc64 sparc64
OK millert
---

diff --git a/usr.sbin/installboot/efi_installboot.c b/usr.sbin/installboot/efi_installboot.c
index eb3c3112467..54142ce3cc9 100644
--- a/usr.sbin/installboot/efi_installboot.c
+++ b/usr.sbin/installboot/efi_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: efi_installboot.c,v 1.4 2022/09/07 10:21:03 kn Exp $	*/
+/*	$OpenBSD: efi_installboot.c,v 1.5 2022/09/09 15:53:16 kn Exp $	*/
 /*	$NetBSD: installboot.c,v 1.5 1995/11/17 23:23:50 gwr Exp $ */
 
 /*
@@ -41,6 +41,7 @@
 #include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
@@ -102,15 +103,11 @@ md_prepareboot(int devfd, char *dev)
 		warnx("disklabel type unknown");
 
 	part = findgptefisys(devfd, &dl);
+	if (part == -1)
+		part = findmbrfat(devfd, &dl);
 	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
-	}
-
-	part = findmbrfat(devfd, &dl);
-	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
+		if (create_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -179,6 +176,8 @@ create_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			return rslt;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+			return -1;
 	}
 
 	return 0;
@@ -232,6 +231,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = create_filesystem(dl, part);
diff --git a/usr.sbin/installboot/i386_installboot.c b/usr.sbin/installboot/i386_installboot.c
index 84dbb8f5899..122bc169b5a 100644
--- a/usr.sbin/installboot/i386_installboot.c
+++ b/usr.sbin/installboot/i386_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: i386_installboot.c,v 1.41 2022/08/31 20:52:15 krw Exp $	*/
+/*	$OpenBSD: i386_installboot.c,v 1.42 2022/09/09 15:53:16 kn Exp $	*/
 /*	$NetBSD: installboot.c,v 1.5 1995/11/17 23:23:50 gwr Exp $ */
 
 /*
@@ -46,6 +46,7 @@
 #include <sys/stat.h>
 #include <sys/sysctl.h>
 #include <sys/time.h>
+#include <sys/wait.h>
 
 #include <ufs/ufs/dinode.h>
 #include <ufs/ufs/dir.h>
@@ -146,8 +147,8 @@ md_prepareboot(int devfd, char *dev)
 
 	part = findgptefisys(devfd, &dl);
 	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
+		if (create_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -280,6 +281,8 @@ create_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			return rslt;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+			return -1;
 	}
 
 	return 0;
@@ -333,6 +336,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = create_filesystem(dl, part);
diff --git a/usr.sbin/installboot/loongson_installboot.c b/usr.sbin/installboot/loongson_installboot.c
index 639cc1001f5..e4ddb6c032b 100644
--- a/usr.sbin/installboot/loongson_installboot.c
+++ b/usr.sbin/installboot/loongson_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: loongson_installboot.c,v 1.4 2021/07/20 14:51:56 kettenis Exp $	*/
+/*	$OpenBSD: loongson_installboot.c,v 1.5 2022/09/09 15:53:16 kn Exp $	*/
 /*	$NetBSD: installboot.c,v 1.5 1995/11/17 23:23:50 gwr Exp $ */
 
 /*
@@ -41,6 +41,7 @@
 #include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
@@ -89,8 +90,8 @@ md_installboot(int devfd, char *dev)
 
 	part = findmbrfat(devfd, &dl);
 	if (part != -1) {
-		write_filesystem(&dl, (char)part);
-		return;
+		if (write_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -143,6 +144,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_EXT2FS, dst, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = snprintf(cmd, sizeof(cmd), newfsfmt,
@@ -157,6 +162,10 @@ write_filesystem(struct disklabel *dl, char part)
 				warn("system('%s') failed", cmd);
 				goto rmdir;
 			}
+			if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+				rslt = -1;
+				goto rmdir;
+			}
 			rslt = mount(MOUNT_EXT2FS, dst, 0, &args);
 			if (rslt == -1) {
 				warn("unable to mount ext2fs partition");
diff --git a/usr.sbin/installboot/macppc_installboot.c b/usr.sbin/installboot/macppc_installboot.c
index 0eb3af5d4f8..fe5c1c71262 100644
--- a/usr.sbin/installboot/macppc_installboot.c
+++ b/usr.sbin/installboot/macppc_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: macppc_installboot.c,v 1.6 2022/09/03 15:46:20 kn Exp $	*/
+/*	$OpenBSD: macppc_installboot.c,v 1.7 2022/09/09 15:53:16 kn Exp $	*/
 
 /*
  * Copyright (c) 2011 Joel Sing <jsing@openbsd.org>
@@ -40,6 +40,7 @@
 #include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
@@ -87,8 +88,8 @@ md_prepareboot(int devfd, char *dev)
 
 	part = findmbrfat(devfd, &dl);
 	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
+		if (create_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -151,6 +152,8 @@ create_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			return rslt;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+			return -1;
 	}
 
 	return 0;
@@ -201,6 +204,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = create_filesystem(dl, part);
diff --git a/usr.sbin/installboot/octeon_installboot.c b/usr.sbin/installboot/octeon_installboot.c
index 950e5bb5f18..7a4f36a3d41 100644
--- a/usr.sbin/installboot/octeon_installboot.c
+++ b/usr.sbin/installboot/octeon_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: octeon_installboot.c,v 1.5 2022/08/31 20:52:15 krw Exp $	*/
+/*	$OpenBSD: octeon_installboot.c,v 1.6 2022/09/09 15:53:16 kn Exp $	*/
 
 /*
  * Copyright (c) 2011 Joel Sing <jsing@openbsd.org>
@@ -40,6 +40,7 @@
 #include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
@@ -85,8 +86,8 @@ md_prepareboot(int devfd, char *dev)
 
 	part = findmbrfat(devfd, &dl);
 	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
+		if (create_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -149,6 +150,8 @@ create_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			return rslt;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+			return -1;
 	}
 
 	return 0;
@@ -202,6 +205,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = create_filesystem(dl, part);
diff --git a/usr.sbin/installboot/powerpc64_installboot.c b/usr.sbin/installboot/powerpc64_installboot.c
index 3e83a1bff44..e56d6abe529 100644
--- a/usr.sbin/installboot/powerpc64_installboot.c
+++ b/usr.sbin/installboot/powerpc64_installboot.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: powerpc64_installboot.c,v 1.4 2022/08/31 20:52:15 krw Exp $	*/
+/*	$OpenBSD: powerpc64_installboot.c,v 1.5 2022/09/09 15:53:16 kn Exp $	*/
 
 /*
  * Copyright (c) 2011 Joel Sing <jsing@openbsd.org>
@@ -40,6 +40,7 @@
 #include <sys/ioctl.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
@@ -87,8 +88,8 @@ md_prepareboot(int devfd, char *dev)
 
 	part = findmbrfat(devfd, &dl);
 	if (part != -1) {
-		create_filesystem(&dl, (char)part);
-		return;
+		if (create_filesystem(&dl, (char)part) == -1)
+			exit(1);
 	}
 }
 
@@ -156,6 +157,8 @@ create_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			return rslt;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+			return -1;
 	}
 
 	return 0;
@@ -210,6 +213,10 @@ write_filesystem(struct disklabel *dl, char part)
 			warn("system('%s') failed", cmd);
 			goto rmdir;
 		}
+		if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+			rslt = -1;
+			goto rmdir;
+		}
 		if (mount(MOUNT_MSDOS, dir, 0, &args) == -1) {
 			/* Try newfs'ing it. */
 			rslt = create_filesystem(dl, part);