From debbugs-submit-bounces@debbugs.gnu.org Mon Mar 12 08:38:56 2018 Received: (at 30604) by debbugs.gnu.org; 12 Mar 2018 12:38:57 +0000 Received: from localhost ([127.0.0.1]:56069 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1evMjA-0002wg-MJ for submit@debbugs.gnu.org; Mon, 12 Mar 2018 08:38:56 -0400 Received: from hera.aquilenet.fr ([185.233.100.1]:50622) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1evMj8-0002wV-Et for 30604@debbugs.gnu.org; Mon, 12 Mar 2018 08:38:55 -0400 Received: from localhost (localhost [127.0.0.1]) by hera.aquilenet.fr (Postfix) with ESMTP id 60BE812382; Mon, 12 Mar 2018 13:38:53 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at aquilenet.fr Received: from hera.aquilenet.fr ([127.0.0.1]) by localhost (hera.aquilenet.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id g2xEDFGgJYeT; Mon, 12 Mar 2018 13:38:51 +0100 (CET) Received: from ribbon (vpn-0-27.aquilenet.fr [IPv6:2a0c:e300:4:27::]) by hera.aquilenet.fr (Postfix) with ESMTPSA id 8A5E211CC1; Mon, 12 Mar 2018 13:38:51 +0100 (CET) From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) To: Danny Milosavljevic Subject: Re: [bug#30604] [PATCH v8 3/7] linux-boot: Load kernel modules only when the hardware is present. References: <20180302153408.14091-1-dannym@scratchpost.org> <20180303135533.6112-1-dannym@scratchpost.org> <20180303135533.6112-4-dannym@scratchpost.org> <87sh9g4vy1.fsf@gnu.org> <20180304133444.4edceecd@scratchpost.org> <87muzgykcl.fsf@gnu.org> <20180309231320.20a02822@scratchpost.org> Date: Mon, 12 Mar 2018 13:38:51 +0100 In-Reply-To: <20180309231320.20a02822@scratchpost.org> (Danny Milosavljevic's message of "Fri, 9 Mar 2018 23:13:20 +0100") Message-ID: <87ina1qxic.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 1.0 (+) X-Debbugs-Envelope-To: 30604 Cc: 30604@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 1.0 (+) Heya Danny, Danny Milosavljevic skribis: > As I said, modprobe mutates /sys - you cannot use find-files. I haven't = used > ftw just to be contrarian. There's even a warning comment :-) I did see this comment, and I don=E2=80=99t doubt you=E2=80=99re acting in = good faith! > In order to find that out, try to print how /sys looked before modprobe -= then > in the early guile recovery REPL print how /sys looks again - the alias it > was juuust complaining about will be there just fine. It wasn=E2=80=99t clear to me what =E2=80=9Cmodprobe mutates /sys=E2=80=9D = meant, and I guess I didn=E2=80=99t think it could lead to not seeing relevant =E2=80=9Cmodalias= =E2=80=9D files. Now I=E2=80=99ve experienced first-hand that it does matter. :-) > It took about 30 h to work out the correct minimal combination - just say= ing :-) Yeah I can imagine (and it would have taken me much longer I guess.) While reviewing this series I realized I didn=E2=80=99t understand everythi= ng, as in the example above, some things seemed unnecessary, and I thought we could improve the style of a few things. The patches that follow build upon your work with a few changes: =E2=80=A2 The =E2=80=9Cmodules.alias=E2=80=9D writer style is made more i= diomatic and hopefully simpler. =E2=80=A2 The =E2=80=9Cmodules.devname=E2=80=9D writer is actually unnece= ssary. I changed it to a more functional style and with smaller functions. It=E2=80=99s the last patch of this series; it=E2=80=99s completely optional since we do= n=E2=80=99t use it currently, but we might want to keep it around for later. =E2=80=A2 The pure-Scheme modprobe uses SRFI-37 instead of (ice-9 getopt-long), as in the rest of the code base. =E2=80=A2 /proc/sys/kernel/modprobe is set in =E2=80=98boot-system=E2=80= =99 directly, so that we don=E2=80=99t have to special-case the =E2=80=9Cmodprobe=E2=80=9D cl= osure in =E2=80=98build-initrd=E2=80=99. =E2=80=A2 =E2=80=98module-aliases->module-file-names=E2=80=99 is gone. I= nstead there=E2=80=99s =E2=80=98load-needed-linux-modules=E2=80=99, similar to the =E2=80=98lo= ad-kernel-modules=E2=80=99 you had, but a bit simpler. =E2=80=A2 I added comments here and there, especially about the virtio-blk-pci, which was far from obvious. :-) I didn=E2=80=99t add the =E2=80=98needed-modules=E2=80=99 and =E2=80=98syst= em-device-aliases=E2=80=99 procedures I posted earlier because we don=E2=80=99t need them currently. If you think they could be useful, we can always add them. The following command succeeds: make check-system TESTS=3D"basic installed-os iso-image-installer" WDYT? I realize that makes for a not-so-efficient review process, though I think the result is worth it. I=E2=80=99m open to suggestions though. Overall, these changes are mostly about (1) making self-contained commits, (2) commenting code, and (3) coding style. About #3, I think the guidelines I applied were: 1. Avoid imperative style. When we do need side effects, try hard to move them separately. 2. Keep functions short. 3. Decompose functions in a way that allows them to be combined =E2=80=9Cnicely=E2=80=9D. Some of that is a bit subjective, but overall we should be able to converge. Thanks for the great work, and sorry for the back-and-forth and delays! Ludo=E2=80=99.