Hi raid5atemyhomework,
On Wed, Jan 06 2021, raid5atemyhomework wrote:
I have this patch below for creating a new
`kernel-loadable-module-service-type`, which can be extended by
another service to add kernel-loadable modules provided by
packages, hope for a review.
I tried out building a VM using this patch and it seemed to work
properly. I was able to add a kernel module by extending
kernel-loadable-module-service-type, and then load it in the
running VM with modprobe.
I only have superficial comments about the patch itself. Hopefully
someone else will provide a better review than I can.
From 984602faba1e18b9eb64e62970147aab653d997f Mon Sep 17
00:00:00 2001
From: raid5atemyhomework <raid5atemyhomew...@protonmail.com>
Date: Tue, 5 Jan 2021 22:27:56 +0800
Subject: [PATCH] gnu: Add 'kernel-loadable-module-service-type'
for services
to extend with kernel-loadable modules.
This will need a proper commit message before being committed.
Guix generally follows the GNU Change Log style[1]. You can see
examples of what commit messages look like by looking at other
commits in the repository.
+;; Only used by the KERNEL-LOADABLE-MODULE-SERVICE-TYPE.
I would remove this comment. This is a description of the current
state, rather than a restriction on how it should be used. As a
description, it might get out of date (if we use
<kernel-builder-configuration> values elsewhere), and it doesn't
add much value (because it can be easily verified, and the record
type isn't exported). If you indent this to be normative, the
comment should explain why this type should not be used elsewhere.
+ (return `(("kernel" ,kernel)
+ ,@(if hurd `(("hurd" ,hurd)) '()))))))
+(define (kernel-builder-configuration-add-modules config
modules)
+ "Constructs a kernel builder configuration that has its
modules extended."
Put empty lines between definitions here ...
+ "Register packages containing kernel-loadable
modules and adds them
+to the system.")))
+(define (kernel-loadable-module-service kernel hurd modules)
+ "Constructs the service that sets up kernel loadable
modules."
... and here.
It would also be worth adding a test to
gnu/tests/linux-modules.scm to test loading modules through this
service.
Other than that, it looks good to me and it seems to work in my
limited testing. 👍 Good job!
Carlo
[1]: https://www.gnu.org/prep/standards/html_node/Change-Logs.html
