git: 2c26d77d989a - main - Remove /boot/efi from mtree, missed in 0b7472b3d8d2.

[Nathan Whitehorn]

On 3/3/21 11:53 AM, Warner Losh wrote:
>

[clipping non-technical pre-history]

>
>     The installer *does* mount the partition in advance, so checking
>     whether
>     there is a mounted file system is a perfectly reasonable test to
>     do. We
>     could also check fstab. I would like to understand what is actually
>     wrong here first, though. Especially after this misfire -- which is
>     problematic for reasons that are still not clear to me, since
>     there are
>     a number of standard directories in hier(7) not in mtree -- I want to
>     make sure we actually do have consensus about what is changing and
>     why.
>
>
> At the top level, we default to having directories in mtree unless 
> there's a good reason not to. We disagree as to whether the installer 
> should take the presence or absence of the directory as a strong 
> enough reason to do something. I don't think that's a good reason.
>
> But leaving that aside, let's say we wanted to reuse the install boot 
> part of the installer to update boot blocks as part of installworld. 
> If we can talk through that example w/o it in mtree, then we can leave 
> it out. The last time I worked through this, though, I thought I'd 
> talked myself into needing it.

> Looking at bootconfig, we could use machdep.bootmethod to determine if 
> we need to update the ESP. If we didn't use that, then the ESP 
> shouldn't be touched. This is, at the moment, x86 centric, but could 
> trivially be added to architectures (I'm happy to add it). This would 
> prevent the 'false positive' that's possible in cases where we've 
> installed UEFI then downgraded to BIOS because of some problem (though 
> purely in the context of the installer, I guess this isn't an issue). 
> Even with your approach, we'd bogusly update an ESP (though one could 
> argue you might want that). We could also change the code so that 
> 'unsupported' architectures just didn't update. This is why I think 
> it's a bit fragile to rely only on the directory being present. It 
> should have something mounted there. If you wanted to mkfs_dos + mkdir 
> efi at the top level, you could check for that directory if you were 
> looking for a flag, though that would still update on a BIOS boot the 
> ESP, and prevent false positives if run as part of an update.

I think we would *want* to update an ESP that is mounted but not 
currently being used. If I set up a dual BIOS/EFI-boot system for some 
reason and happened to install an update while booted from BIOS, I would 
be deeply astonished if my configured-by-the-installer EFI bootloader 
did not also get updated.

(As an aside, I would also much rather the installer use an update 
utility to set up the ESP than have the update utility use the installer.)

So here's a proposal, now that everyone is in the CC list:
- We add /boot/efi back to mtree, even though I find it kind of weird to 
have it there I think we're too close to the release to have a 
conclusion on this.
- We have the installer check for either the ESP directory being an 
active mountpoint or being in the in-progress fstab, whichever is 
easiest to implement (they are equivalent for the installer).

If that seems OK, I'll post another review for the change.

> A long-term project I've had has been to try to update the boot blocks 
> as part of installworld or maybe as part of installboot. We have 
> really poor reuse as a project in this area. Every little 
> orchestration thing wrote its own thing, and all of them have done it 
> badly. I was hoping to be able to reuse this code, or modify the 
> installer to use whatever we come up with there. As part of that, I 
> had talked myself into thinking we always needed /boot/efi, but I'm 
> having trouble reconstructing why that is now though I know it had to 
> do with installed systems and bootstrapping issues... I know I was 
> worried about questions about 'why isn't /boot/efi on the system by 
> default so I can mount it' for people that have upgraded, but I recall 
> there was more to it than that. With it in mtree, an installworld 
> (even w/o an ESP update) would create it and people could mount it w/o 
> having to mkdir which they might make as $SOMETHING_ELSE. So I guess 
> that's a bit of a weak reason to absolutely require it in mtree.

Thanks a lot for the explanation. I'm agreed entirely about the problem 
and the difficulty -- hopefully this set of changes helps at least.

As for mtree, I was imagining this as something like /home, which is a 
standard part of the system but isn't part of mtree since it depends on 
local-system policy. It's also different from /home in that we *do* want 
it to be a standard place for updates, of course. I think there's really 
not a ton of precedent either way: we don't have any other mount points 
in there for file systems that may or may not exist depending on 
circumstances, as far as I can tell. My worry with having it in mtree is 
that having it exist but potentially be a directory rather than an 
actual ESP requires that update tools be a little smarter and errors 
will be a little less obvious, since updates that don't pay enough 
attention will be a bit more likely to splat files there assuming there 
is an ESP even if makes no sense. It's a weak consideration either way, 
I think.
-Nathan

>
> Warner