GNU bug report logs - #30604
[PATCH 0/4] Load Linux module only when supported hardware is present.

Previous Next

Full log

Message #272 received at 30604 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 30604 <at> debbugs.gnu.org
Subject: Re: [bug#30604] [PATCH v8 3/7] linux-boot: Load kernel modules only
 when the hardware is present.
Date: Mon, 12 Mar 2018 13:38:51 +0100

Heya Danny,

Danny Milosavljevic <dannym <at> scratchpost.org> 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’t doubt you’re 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’t clear to me what “modprobe mutates /sys” meant, and I guess I
didn’t think it could lead to not seeing relevant “modalias” files.  Now
I’ve experienced first-hand that it does matter.  :-)

> It took about 30 h to work out the correct minimal combination - just saying :-)

Yeah I can imagine (and it would have taken me much longer I guess.)

While reviewing this series I realized I didn’t understand everything,
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:

  • The “modules.alias” writer style is made more idiomatic and
    hopefully simpler.

  • The “modules.devname” writer is actually unnecessary.  I changed it
    to a more functional style and with smaller functions.  It’s the
    last patch of this series; it’s completely optional since we don’t
    use it currently, but we might want to keep it around for later.

  • The pure-Scheme modprobe uses SRFI-37 instead of (ice-9
    getopt-long), as in the rest of the code base.

  • /proc/sys/kernel/modprobe is set in ‘boot-system’ directly, so that
    we don’t have to special-case the “modprobe” closure in
    ‘build-initrd’.

  • ‘module-aliases->module-file-names’ is gone.  Instead there’s
    ‘load-needed-linux-modules’, similar to the ‘load-kernel-modules’
    you had, but a bit simpler.

  • I added comments here and there, especially about the
    virtio-blk-pci, which was far from obvious.  :-)

I didn’t add the ‘needed-modules’ and ‘system-device-aliases’ procedures
I posted earlier because we don’t need them currently.  If you think
they could be useful, we can always add them.

The following command succeeds:

  make check-system TESTS="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’m 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
     “nicely”.

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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.