Re: ZFS on Guix

[Carlo Zancanaro <carlo@zancanaro.id.au>]

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 <raid5atemyhomework@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