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;

Reply via email to