From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1evMjL-00080N-HX for guix-patches@gnu.org; Mon, 12 Mar 2018 08:39:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1evMjG-000526-I7 for guix-patches@gnu.org; Mon, 12 Mar 2018 08:39:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:48172) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1evMjG-00051N-DB for guix-patches@gnu.org; Mon, 12 Mar 2018 08:39:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1evMjG-0002x0-0V for guix-patches@gnu.org; Mon, 12 Mar 2018 08:39:02 -0400 Subject: [bug#30604] [PATCH v8 3/7] linux-boot: Load kernel modules only when the hardware is present. Resent-Message-ID: From: ludo@gnu.org (Ludovic =?UTF-8?Q?Court=C3=A8s?=) 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Danny Milosavljevic Cc: 30604@debbugs.gnu.org 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.