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.
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?
write_filesystem() system(3)ing fsck_msdos(8) has the same problem and
ought to be fixed as well.
Index: efi_installboot.c
===================================================================
RCS file: /mount/openbsd/cvs/src/usr.sbin/installboot/efi_installboot.c,v
retrieving revision 1.2
diff -u -p -r1.2 efi_installboot.c
--- efi_installboot.c 3 Feb 2022 10:25:14 -0000 1.2
+++ efi_installboot.c 25 Aug 2022 08:30:11 -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>
@@ -100,16 +103,11 @@ md_prepareboot(int devfd, char *dev)
warnx("disklabel type unknown");
part = findgptefisys(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 (part == -1)
+ part = findmbrfat(devfd, &dl);
+ if (part != -1)
+ if (create_filesystem(&dl, (char)part) == -1)
+ exit(1);
}
void
@@ -177,6 +175,9 @@ create_filesystem(struct disklabel *dl,
warn("system('%s') failed", cmd);
return rslt;
}
+
+ if (WIFEXITED(rslt) && WEXITSTATUS(rslt))
+ return -1;
}
return 0;