On Sat, Sep 03, 2022 at 12:28:33PM +0000, Klemens Nanni wrote:
> On Fri, Aug 26, 2022 at 09:12:59AM +0000, Klemens Nanni wrote:
> > installboot(8) runs newfs_msdos(8) via system(3) but only checks failures
> > of the function itself, always returning zero no matter what newfs_msdos
> > returned.
> >
> > This is bad for regress tests relying on correct return codes.
>
> Also the installer uses 'install -p' and will do so on even more
> architectures soon.
>
> It's invocations in distrib/$(machine)/ramdisk/install.md are currently
> unchecked, but even if we used 'set -e' of
> if ! installboot -p $_disk ; then
> ...
> fi
>
> they would not have any effect as -p always reports success upon
> fsck/newfs failure.
>
> >
> > 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_msdos fails because of the
> > vnd/disklabel race, see "Race in disk_attach_callback?" on tech@):
> >
> > # installboot -vp $(<obj/vnd2.txt) ; 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
> >
> > Feedback?
> >
> > If this direction seems OK, I'll fix all other MD specific copies of this
> > code.
> >
> > It seems like write_filesystem() is identical across all MD copies,
> > so maybe it could even be merged into a common filesystem.c and be fixed
> > once for all of them?
>
> Anyone?
>
> >
> > write_filesystem() system(3)ing fsck_msdos(8) has the same problem and
> > ought to be fixed as well.
Here's the full diff to fsck and newfs failure on all architectures.
This is a regular programming issue around system(3) that has nothing to
do with bootloaders or installboot per se.
I'd be great to get this in to make further progress, especially on
softraid support for the -p option. The less diffs I have to juggle on
multiple test machines the better.
Feedback? OK?
Index: efi_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
retrieving revision 1.4
diff -u -p -r1.4 efi_installboot.c
--- efi_installboot.c 7 Sep 2022 10:21:03 -0000 1.4
+++ efi_installboot.c 8 Sep 2022 19:54:43 -0000
@@ -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,
warn("system('%s') failed", cmd);
return rslt;
}
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;
@@ -230,6 +229,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
Index: i386_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/i386_installboot.c,v
retrieving revision 1.41
diff -u -p -r1.41 i386_installboot.c
--- i386_installboot.c 31 Aug 2022 20:52:15 -0000 1.41
+++ i386_installboot.c 8 Sep 2022 19:55:02 -0000
@@ -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,
warn("system('%s') failed", cmd);
return rslt;
}
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;
@@ -331,6 +334,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
Index: loongson_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/loongson_installboot.c,v
retrieving revision 1.4
diff -u -p -r1.4 loongson_installboot.c
--- loongson_installboot.c 20 Jul 2021 14:51:56 -0000 1.4
+++ loongson_installboot.c 8 Sep 2022 19:54:48 -0000
@@ -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, c
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,
@@ -155,6 +160,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
rslt = mount(MOUNT_EXT2FS, dst, 0, &args);
Index: macppc_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/macppc_installboot.c,v
retrieving revision 1.6
diff -u -p -r1.6 macppc_installboot.c
--- macppc_installboot.c 3 Sep 2022 15:46:20 -0000 1.6
+++ macppc_installboot.c 8 Sep 2022 19:57:45 -0000
@@ -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,
warn("system('%s') failed", cmd);
return rslt;
}
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;
@@ -199,6 +202,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
Index: octeon_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/octeon_installboot.c,v
retrieving revision 1.5
diff -u -p -r1.5 octeon_installboot.c
--- octeon_installboot.c 31 Aug 2022 20:52:15 -0000 1.5
+++ octeon_installboot.c 8 Sep 2022 19:58:47 -0000
@@ -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,
warn("system('%s') failed", cmd);
return rslt;
}
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;
@@ -200,6 +203,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
if (mount(MOUNT_MSDOS, dst, 0, &args) == -1) {
Index: powerpc64_installboot.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/powerpc64_installboot.c,v
retrieving revision 1.4
diff -u -p -r1.4 powerpc64_installboot.c
--- powerpc64_installboot.c 31 Aug 2022 20:52:15 -0000 1.4
+++ powerpc64_installboot.c 8 Sep 2022 19:55:04 -0000
@@ -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,
warn("system('%s') failed", cmd);
return rslt;
}
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;
@@ -208,6 +211,10 @@ write_filesystem(struct disklabel *dl, c
rslt = system(cmd);
if (rslt == -1) {
warn("system('%s') failed", cmd);
+ goto rmdir;
+ }
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt)) {
+ rslt = -1;
goto rmdir;
}
if (mount(MOUNT_MSDOS, dir, 0, &args) == -1) {
