On Mon, Nov 21, 2022 at 07:01:44PM +0100, Tobias Heider wrote:
> On Mon, Nov 21, 2022 at 03:09:25PM +0000, Klemens Nanni wrote:
> > 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.
>
> One more version with your comments addressed.
> better?
One unaddressed nit remains.
OK kn, assuming that you've sufficiently tested both apple and non-apple
use cases alike.
>
> 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 17:58:19 -0000
> @@ -70,6 +70,7 @@
>
> static int create_filesystem(struct disklabel *, char);
> static void write_filesystem(struct disklabel *, char);
> +static int write_firmware(const char *, const 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';
> + rslt = write_firmware(root, dst);
> + if (rslt == -1) {
> + warnx("unable to write firmware");
> + goto umount;
> + }
> +
> rslt = 0;
This line is still redundant as pointed out earlier.
rslt is zero if write_firmware() worked and -1 if not, in which case
the code jumps to unmount right away... which is the next line, anyway.
With 'rslt = 0;' gone, you can leave the goto to keep the usual idiom in
tact, or you can simply do
dst[mntlen] = '\0';
rslt = write_firmware(root, dst);
if (rslt == -1) {
warnx("unable to write firmware");
unmount:
...
which is what the code will do either way.
>
> umount:
> @@ -325,6 +333,61 @@ rmdir:
>
> if (rslt == -1)
> exit(1);
> +}
> +
> +static int
> +write_firmware(const char *root, const char *mnt)
> +{
> + char dst[PATH_MAX];
> + char fw[PATH_MAX];
> + char *src;
> + struct stat st;
> + int rslt;
> +
> + 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) {
> + warnx("unable to build /etc/firmware path");
> + 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;
> + warnx("unable to build /m1n1 path");
> + goto cleanup;
> + }
> + 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;
> + warnx("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;
> }
>
> /*
>
