Package: guix-patches;
Reported by: Sisiutl <sisiutl <at> egregore.fun>
Date: Sun, 6 Oct 2024 09:45:01 UTC
Severity: normal
Tags: moreinfo, patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #17 received at 73654 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: soeren <at> soeren-tempel.net Cc: 73654 <at> debbugs.gnu.org, sisiutl <at> egregore.fun, ludo <at> gnu.org, hako <at> ultrarare.space Subject: Re: [bug#73654] [PATCH v2] mapped-devices: luks: Support passing --allow-discards during open Date: Mon, 10 Mar 2025 11:49:56 +0900
Hi,
soeren <at> soeren-tempel.net writes:
> From: Sisiutl <sisiutl <at> egregore.fun>
>
> * gnu/system/mapped-devices.scm (open-luks-device): Support opening
> LUKS devices with the --allow-discards option.
> * gnu/system/mapped-devices.scm (luks-device-mapping-with-options):
> Pass through the allow-discards? keyword argument.
> * doc/guix.texi (Mapped Devices): Update documentation for the
> luks-device-mapping-with-options procedure.
>
> Signed-off-by: Sören Tempel <soeren <at> soeren-tempel.net>
I'd use a 'Co-authored-by' if significantly modified or 'Modified-by' if
lightly touched git trailers here. Signed-off-by is currently used in
Guix to denote someone else's work pushed by a committer.
> ---
> Not the author of the original patchset, but I needed this for my
> own setup as well so I might as well pick up the slack. I made
> the following changes since the v1:
>
> * Mention allow-discards? in the docstring of open-luks-device.
> * Reference the new option in luks-device-mapping-with-options.
> * Expand the related documentation in doc/guix.texi.
> * Revise the commit message slightly.
> * Restore the linefeed.
Sounds good.
> doc/guix.texi | 11 +++++++++-
> gnu/system/mapped-devices.scm | 39 ++++++++++++++++++++---------------
> 2 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 05c855c5ea..bc3ba1f2ed 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -18461,7 +18461,7 @@ Mapped Devices
> @code{dm-crypt} Linux kernel module.
> @end defvar
>
> -@deffn {Procedure} luks-device-mapping-with-options [#:key-file]
> +@deffn {Procedure} luks-device-mapping-with-options [#:key-file #:allow-discards?]
> Return a @code{luks-device-mapping} object, which defines LUKS block
> device encryption using the @command{cryptsetup} command from the
> package with the same name. It relies on the @code{dm-crypt} Linux
> @@ -18483,6 +18483,15 @@ Mapped Devices
> (type (luks-device-mapping-with-options
> #:key-file "/crypto.key")))
> @end lisp
> +
> +If @code{allow-discards?} is provided, then the use of discard (TRIM)
> +requests is allowed for the underlying device.
I'd streamline this sentence into:
--8<---------------cut here---------------start------------->8---
@code{allow-discards?} allows the use of discard (TRIM) requests for the
underlying device.
--8<---------------cut here---------------end--------------->8---
> + This is useful for
> +Solid State Drives.
I'd use 'solid state drives', un-capitalized or @acronym{SSD, Solid
State Drives}.
> However, this option can have a negative security
> +impact because it can make filesystem-level operations visible on the
The GNU convention is to use 'file system', not filesystem.
> +physical device. For more information, refer to the description of
> +the @code{--allow-discards} option in the @code{cryptsetup-open(8)}
> +man page.
> +
> @end deffn
>
> @defvar raid-device-mapping
> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
> index 931c371425..c3eaf9ff6e 100644
> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -194,9 +194,10 @@ (define (check-device-initrd-modules device linux-modules location)
> ;;; Common device mappings.
> ;;;
>
> -(define* (open-luks-device source targets #:key key-file)
> +(define* (open-luks-device source targets #:key key-file allow-discards?)
> "Return a gexp that maps SOURCE to TARGET as a LUKS device, using
> -'cryptsetup'."
> +'cryptsetup'. When ALLOW-DISCARDS? is true, the use of discard (TRIM) requests is
> +allowed for the underlying device."
> (with-imported-modules (source-module-closure
> '((gnu build file-systems)
> (guix build utils))) ;; For mkdir-p
> @@ -234,17 +235,19 @@ (define* (open-luks-device source targets #:key key-file)
> (loop (- tries-left 1))))))
> (error "LUKS partition not found" source))
> source)))
> - ;; We want to fallback to the password unlock if the keyfile fails.
> - (or (and keyfile
> - (zero? (system*/tty
> - #$(file-append cryptsetup-static "/sbin/cryptsetup")
> - "open" "--type" "luks"
> - "--key-file" keyfile
> - partition #$target)))
> - (zero? (system*/tty
> - #$(file-append cryptsetup-static "/sbin/cryptsetup")
> - "open" "--type" "luks"
> - partition #$target)))))))))
> + (let* ((cryptsetup-flags (list "open" "--type" "luks" partition #$target))
> + (cryptsetup-flags (if allow-discards?
> + (cons "--allow-discards" cryptsetup-flags)
> + cryptsetup-flags)))
Theres' not need for a let* and reusing the same variable; you can
instead use the following list splicing trick:
--8<---------------cut here---------------start------------->8---
(let ((options `(,@(if allow-discards?
"--allow-discards"
'())
"open" "--type" "luks" partition #$target)))
[...])
--8<---------------cut here---------------end--------------->8---
> + ;; We want to fallback to the password unlock if the keyfile fails.
> + (or (and keyfile
> + (zero? (apply system*/tty
> + #$(file-append cryptsetup-static "/sbin/cryptsetup")
> + "--key-file" keyfile
> + cryptsetup-flags)))
> + (zero? (apply system*/tty
> + #$(file-append cryptsetup-static "/sbin/cryptsetup")
> + cryptsetup-flags))))))))))
You'll want to nest the apply under the (zero? ... call and ensure it
fits under 80 characters, which is in our coding style guidelines.
> (define (close-luks-device source targets)
> "Return a gexp that closes TARGET, a LUKS device."
> @@ -286,13 +289,15 @@ (define luks-device-mapping
> ((gnu build file-systems)
> #:select (find-partition-by-luks-uuid system*/tty))))))
>
> -(define* (luks-device-mapping-with-options #:key key-file)
> +(define* (luks-device-mapping-with-options #:key key-file allow-discards?)
> "Return a luks-device-mapping object with open modified to pass the arguments
> -into the open-luks-device procedure."
> +(key-file and allow-discards?) into the open-luks-device procedure."
I would drop the above doc change. 'Arguments' already cover it in a
more abstract (and maintainable) fashion.
> (mapped-device-kind
> (inherit luks-device-mapping)
> - (open (λ (source targets) (open-luks-device source targets
> - #:key-file key-file)))))
> + (open (λ (source targets)
> + (open-luks-device source targets
> + #:key-file key-file
> + #:allow-discards? allow-discards?)))))
The rest LGTM. Could you please send a new revision taking into account
my review comments?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.