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.
>
>
> 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;
>
