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

[Rodney W. Grimes]

> On Thu, 4 Mar 2021 06:56:11 -0800 (PST)
> "Rodney W. Grimes" <freebsd at gndrsh.dnsmgr.net> wrote:
> 
> > > On 3/3/21 10:38 AM, Warner Losh wrote:
> > > >
> > > >
> > > > On Wed, Mar 3, 2021 at 7:13 AM Nathan Whitehorn 
> > > > <nwhitehorn at freebsd.org <mailto:nwhitehorn at freebsd.org>> wrote:
> > > >
> > > >
> > > >
> > > >     On 3/3/21 9:05 AM, Brandon Bergren wrote:
> > > >     > On Wed, Mar 3, 2021, at 6:53 AM, Rodney W. Grimes wrote:
> > > >     >> What am I missing here?? One place I am being told this is run in
> > > >     >> an environment that may not even be an EFI booted system, and in
> > > >     >> another place it is being used as a test if something is mounted
> > > >     >> on it, which should only be true on an EFI booted system.
> > > >     > That the script in question is a generic script that runs as
> > > >     part of bsdinstall on every platform and has to be universal.
> > > >     >
> > > >     > The actual *problem* here is that
> > > >     usr.sbin/bsdinstall/scripts/bootconfig has a default case that is
> > > >     >? ? ? ? ? ? ? *)? ? ? ? ? die "Unsupported arch $(uname -m) for
> > > >     UEFI install"
> > > >     >
> > > >     > which then causes the main script to bail out, leaving the
> > > >     system in a half-installed state.
> > > >     >
> > > >     > If that had just been an exit 0 this would have never been a
> > > >     problem, I suppose.
> > > >     >
> > > >     > Before the original change that broke this, there was a check
> > > >     that the script was not running on powerpc or mips platforms
> > > >     before running the efi bits, but this got taken out.
> > > >     >
> > > >
> > > >     Well, incidentally. The bootconfig script needs to know if there
> > > >     is an
> > > >     ESP it should configure, but the signalling mechanism (the
> > > >     presence of
> > > >     the ESP mount point) was being broken by mtree making that directory
> > > >     unconditionally even on systems that don't use EFI. So then
> > > >     bootconfig
> > > >     tried to set it up, but failed later on, because there was no EFI
> > > >     loader
> > > >     to set up. The mtree change makes the ESP mount point only exist on
> > > >     systems with an ESP.
> > > >
> > > >
> > > > So you made a unilateral change, without discussion of the bigger 
> > > > design, to something without even asking the original person who made 
> > > > the change to mtree about it for what sounds like an obscure case in 
> > > > the installer that could be solved in a different way? It's trivial 
> > > > enough to look at the boot method sysctl and skip the EFI update if we 
> > > > didn't boot EFI (and if by change that's not on all systems, it's easy 
> > > > enough to add it on all systems). I have no notion about why that 
> > > > wasn't considered, at least, before jumping in and taking people by 
> > > > surprise.
> > 
> > I still do not understand why machdep.bootmethod=EFI was rejected?
> > Is this value not present on ALL platforms that boot in EFI mode?
> > if exist(machdep.bootmethod) && machdep.bootmethod=EFI seems to
> > me to be the best and valid way to make this decision.  If that
> > has issues working on a platform we need to fix that issue and not
> > do all this other stuff.
> 
>  We need to install and create the efi dir even if the installer is
> booted in CSM mode, so a user can switch to full uefi mode after and
> still can boot the FreeBSD that was installed. (The same thing must be
> done for bios boot code).

Ah, yes, ok, I see that issue, but isnt that driven by the fact
the user has selected GPT (EFI) in the disk menu, so could be
drive by an installer variable like any other aspect of the
installer?  Passing around the users install parameters via
the file system is fragile as noted else where, this information
should be clearly avaliable within the installer script itself.

> 
> > > >
> > > > Next time, talk to people first. That's the whole point of having 
> > > > review tools, mailing list and git blame.
> > > >
> > > > Warner
> > > 
> > > This method of testing was in the original review here posted on Feb. 
> > > 23: https://reviews.freebsd.org/D28897
> > > 
> > > The description of the test procedure you're objecting to was even in 
> > > the summary! Then we had a discussion by email about the change to mtree 
> > > on the committers list on Feb. 28 to resolve a bug affecting PowerPC in 
> > > the patch reviewed and approved by you. I then waited several days and 
> > > had a long thread for several days on the mailing list about the 
> > > approach. coming up with this short patch -- again, as a bug fix to a 
> > > reviewed approach.
> > > 
> > > We can change the logic -- that's fine! But, to paraphrase, the reason 
> > > we have reviews is so people like you can look at the review and note 
> > > these kinds of problems when they are reviewed, not after the commit 
> > > goes in. There's a significant amount of whiplash when you do get 
> > > patches reviewed, approved, and then the person who reviewed and 
> > > approved them accuses you of "taking people by surprise".
> > > 
> > > 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.
> > 
> > These *should* be fixed.  ALL directories that are part of a finished
> > FreeBSD system should be present both in hier.7 and in the mtree files,
> > deviating from that should only be allowed if there is some really
> > really grand reasons.  If the reason is "architecture foo does not
> > have directory /bar/zap" one could and should create an arch specific
> > mtree file that covers these, having these mkdir's (prefer to see
> > all those changed to install -d's) scattered around all over, IMHO
> > creates maintance and inconsistency issues.
> > 
> > > -Nathan
> > -- 
> > Rod Grimes                                                 rgrimes at freebsd.org
> -- 
> Emmanuel Vadot <manu at bidouilliste.com> <manu at FreeBSD.org>
-- 
Rod Grimes                                                 rgrimes at freebsd.org