On Mon, Nov 21, 2022 at 03:42:37PM +0100, Tobias Heider wrote:
> Here is a more cleaned up version of the previous diff. I moved all the
> firmware logic to a new write_firmware() function. This should be easy
> to extend if we decide to ship more firmware this way.
This seems more tidy.
>
> The diff passes regress and manual tests with and without $ESP/m1n1/,
> /etc/firmware and /etc/firmware/apple-boot.bin.
Reads good, but I haven't compile- or run-tested it.
Comments inline.
>
> ok?
>
> Index: efi_installboot.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/installboot/efi_installboot.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 efi_installboot.c
> --- efi_installboot.c 6 Nov 2022 12:33:41 -0000 1.7
> +++ efi_installboot.c 21 Nov 2022 14:21:29 -0000
> @@ -70,6 +70,7 @@
>
> static int create_filesystem(struct disklabel *, char);
> static void write_filesystem(struct disklabel *, char);
> +static int write_firmware(char *, char *);
> static int findgptefisys(int, struct disklabel *);
> static int findmbrfat(int, struct disklabel *);
>
> @@ -308,6 +309,13 @@ write_filesystem(struct disklabel *dl, c
> goto umount;
> }
>
> + dst[mntlen] = '\0';
> + if (write_firmware(root, dst) == -1) {
> + warn("unable to write firmware");
> + rslt = -1;
> + goto umount;
> + }
> +
write_firmware() follows the 0/-1 idiom and the following would make
that immediately obvious:
rslt = write_firmware();
if (rslt == -1) {
warnx();
goto unmount;
}
> rslt = 0;
Then this line can also go.
>
> umount:
> @@ -325,6 +333,61 @@ rmdir:
>
> if (rslt == -1)
> exit(1);
> +}
> +
> +static int
> +write_firmware(char *root, char *mnt)
Both arguments are each only read once, so they can be const.
> +{
> + char dst[PATH_MAX];
> + char fw[PATH_MAX];
> + char *src;
> + struct stat st;
> + int rslt;
> +
> + src = NULL;
> + strlcpy(dst, mnt, sizeof(dst));
> +
> + /* Skip if no /etc/firmware exists */
> + rslt = snprintf(fw, sizeof(fw), "%s/%s", root, "etc/firmware");
> + if (rslt < 0 || rslt >= PATH_MAX) {
> + warn("unable to build /etc/firmware path");
I don't think this should print errno.
Most of these "unable to X" warnings already say everything the user
needs to know and thus use warnx(); it seems that efi_installboot.c has
a few warn() by mistake, but I'd have to double check on this.
> + return -1;
> + }
> + if ((stat(fw, &st) != 0) || !S_ISDIR(st.st_mode))
> + return 0;
> +
> + /* Copy apple-boot firmware to /m1n1/boot.bin if available */
> + src = fileprefix(fw, "/apple-boot.bin");
> + if (src == NULL)
> + return -1;
> + if (access(src, R_OK) == 0) {
> + if (strlcat(dst, "/m1n1", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1 path");
> + goto cleanup;
Nit: the order is inconsistent with the rest of installboot, which does
warnx();
rslt = -1;
goto somewhere;
> + }
> + if ((stat(dst, &st) != 0) || !S_ISDIR(st.st_mode)) {
> + rslt = 0;
> + goto cleanup;
> + }
> + if (strlcat(dst, "/boot.bin", sizeof(dst)) >= sizeof(dst)) {
> + rslt = -1;
> + warn("unable to build /m1n1/boot.bin path");
> + goto cleanup;
> + }
> + if (verbose)
> + fprintf(stderr, "%s %s to %s\n",
> + (nowrite ? "would copy" : "copying"),
> + src, dst);
> + if (!nowrite)
> + rslt = filecopy(src, dst);
> + if (rslt == -1)
> + goto cleanup;
> + }
> + rslt = 0;
> + cleanup:
> + free(src);
> + return rslt;
> }
>
> /*
>
