Mark Kettenis <[email protected]> wrote:
> > From: "Theo de Raadt" <[email protected]>
> > Date: Tue, 01 Feb 2022 09:28:59 -0700
> >
> > A few comments.
> >
> > I think this approach is increasingly fragile, because a change for one
> > architecture will affect others.
> >
> > > -.elif ${MACHINE} == "armv7" || ${MACHINE} == "arm64"
> > > +.elif ${MACHINE} == "armv7" || ${MACHINE} == "arm64" || ${MACHINE} ==
> > > "riscv64"
> > > SRCS += armv7_installboot.c
> >
> > How about copying armv7_installboot.c to a new file, and then it even
> > becomes
> > possible to perform this operation in the C code also:
> >
> > > echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh
> >
> > As a bonus, it will umount the disk... which the current shell function
> > forgets
> > to do.
> >
> > I think a new file is better, if this addition is made, rather than hiding
> > it
> > behind a #ifdef inside a .if file
>
> I disagree. Apart from the name of the bootloader file, the code is
> identical and this is by design!
>
> Maybe we should rename the file to efi_installboot.c and/or rearrange
> the code slightly such that is becomes more obvious that the name of
> the file is indeed the only difference.
The code cannot be the same, because installboot is supposed to do the
*COMPLETE OPERATION* of making a disk bootable.
But with the proposal, installboot(8) fails to do the complete job, and
the md_installboot() shell function has to still do:
echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh
But look closer. md_intallboot does a mount also.
But if this isn't changed, the actual sequence really becomes:
newfs -t msdos
mount
copy file
unmount
mount
echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh
unmount
Right now, the operation is:
newfs -t msdos (elsewhere)
mount
copy file
echo bootriscv64.efi > /mnt/mnt/efi/boot/startup.nsh
unmount # it actually forgets to do this
Really, that is what the operations in md_installboot() script expands to.
Right now, it mounts once. With the proposal, it mounts the filesystem twice.
That is pretty ridiculous.
The way that gets solved is to put the startup.nsh file creation directly into
installboot(8). Into armv7_installboot.c. But obviously hiding behind an
#ifdef __riscv64__ or something? Gross isn't it.
So on the balance, copying the code is a better plan.
Most of the *_installboot.c files already have a huge amount of duplication,
mixed with little changes which we don't hide behind #ifdef. Hey, we could
merge all of *_installboot.c into one file, with a pile of #ifdef. BUT
that is what we used to have, and it was difficult to maintain correctness.
The new instalboot(8) design is such that you run the command and it does
EVERYTHING. Visa's proposal is pointless if the filesystem still has to be
mounted a 2nd time to create a file, because of some purist attitude that
this code should be shared.
