From owner-dev-commits-src-main@freebsd.org Wed Mar 3 16:53:56 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 00805548C57 for ; Wed, 3 Mar 2021 16:53:56 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DrKpC65zPz4YXp for ; Wed, 3 Mar 2021 16:53:55 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x72e.google.com with SMTP id l4so9868518qkl.0 for ; Wed, 03 Mar 2021 08:53:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=z1TFELr0rieE1fJITpeppTIQj7JRe8DaH221JOdljoI=; b=DRp+uaH70TsfNpkCd70PEDvA7BoeMOkh44EK2Lh85Sej3jdmAYaPBqyAYW2DLO0Mtb 5VcJ/XOlYFQpJQlyltJ+cYv3fv927koJmTxomKdJQDwo/lgPV1bs+Pe8wQ9m/hg2jrch x0bgXqDeX7mL39t0DXO4Cp/pzQuKOcm8+Hf0AvK4IQOiQRgPQ8GlWWMcE1tSHPYVT64A mq/9VW9mCESKmSY29zey/yR0ajp+QB0w1FMuQLPn1maMOxVwP5jZwT6JD7Vjbtqvt6AI stvMTiBvL/pNhcmwapqn6VCia0KNS3IAqh+2E2SbOSo3bB8dXoTjwAnjdmOj+NF2SlXP Qn1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=z1TFELr0rieE1fJITpeppTIQj7JRe8DaH221JOdljoI=; b=fAHP8w64gStv2a1gUCz6N9tQx4o87QFZX11q3G4hDURYQuTcPZ7hHe5YrQBglz4qgY NgmYMl1cnNtaORPtDhutfpaYw3Q51z9wCvUK31rmOeMvpmGGWTyR333TnJYLkIc4VlwZ VKCXFu57awQoB4QqZ0ZaR/IvAtL+zQoKqRKFRu81Zu6bTCJNMY2KI7+2uUKg1CSoDvBM Lf7ZQIJ9rNstec+cgLjUKZxcDpi2FeegY98kY414fM239y+aA7cxHNH4WSiOP4Z8/AT0 KKX/3X1fZOtPfGhQSFxmkxrf4g1GjDyrBB6coRvMe3RD5oBzIz83VN59aIdYvKuE5SNz yAgA== X-Gm-Message-State: AOAM5315xNBJz/541ZpjhlGpsTL69aTzcK8LAcqPYCUdpUpRHLwPyRFP ans1uy247j/yBoKzJHYnraWiX7zkcZh0ElV2cOYKpw== X-Google-Smtp-Source: ABdhPJzymoQ/QZfJpFMPSy6kfJbJcoJAj7I1E74EDWa7eEGWTTsGhI55SN2/8WIj6VqCHU3GJ7/zUsSgGfrXgVPeT6Y= X-Received: by 2002:a37:a085:: with SMTP id j127mr24301369qke.206.1614790434739; Wed, 03 Mar 2021 08:53:54 -0800 (PST) MIME-Version: 1.0 References: <202103031253.123CrxKG051357@gndrsh.dnsmgr.net> <14d09680-1036-4a7e-8a0e-c3063cac8bc9@www.fastmail.com> <6e52fee6-a2fd-584f-757e-e77a8f8ea8eb@freebsd.org> In-Reply-To: <6e52fee6-a2fd-584f-757e-e77a8f8ea8eb@freebsd.org> From: Warner Losh Date: Wed, 3 Mar 2021 09:53:43 -0700 Message-ID: Subject: Re: git: 2c26d77d989a - main - Remove /boot/efi from mtree, missed in 0b7472b3d8d2. To: Nathan Whitehorn Cc: Brandon Bergren , "Rodney W. Grimes" , Ed Maste , src-committers , "" , dev-commits-src-main@freebsd.org X-Rspamd-Queue-Id: 4DrKpC65zPz4YXp X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.34 X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Mar 2021 16:53:56 -0000 On Wed, Mar 3, 2021 at 8:55 AM Nathan Whitehorn wrote: > > > On 3/3/21 10:38 AM, Warner Losh wrote: > > > > > > On Wed, Mar 3, 2021 at 7:13 AM Nathan Whitehorn > > > 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. > > > > 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 That review didn't remove it from mtree, though... > > 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 mtree stuff was a surprise. It wasn't in the review. It was a big surprise in an area that had been carefully negotiated over a year ago, so I had a strong reaction. Especially when git blame shows I did the commit, I'd have expected at least a 'hey, why is this here?' to come up... But rather than rehashing here, I'd like to focus on the technical side. 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. 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. Warner