luks-device-mapping-with-options breaks bootloader

  • Open
  • quality assurance status badge
Details
4 participants
  • 45mg
  • Ludovic Courtès
  • Tadhg McDonald-Jensen
  • Tomas Volf
Owner
unassigned
Submitted by
Tadhg McDonald-Jensen
Severity
important

Debbugs page

T
T
Tadhg McDonald-Jensen wrote on 7 May 2024 11:54
(address . bug-guix@gnu.org)
CAP5DvDj1Xgkr5Mzy2XEzj6O86Nm=Z=hczhKQpCYKhV8Z7mg=BA@mail.gmail.com
using the `luks-device-mapping-with-options` mapped device type defined in
(gnu system mapped-devices) causes grub or other bootloaders to not
properly attempt to mount the encrypted drive. This is caused by the
commit 39a9404 which identifies luks mapped devices by checking if the type
is equal to `luks-device-mapping`, so by using a different routine that is
a proxy to that one it doesn't forward it to grub in the
store-crypto-devices list.

For anyone who finds this before it is fixed, you can boot your device by
hitting 'c' in grub and typing these commands:
grub> insmod luks
grub> insmod luks2
grub> cryptomount (XXX)
grub> set root=(crypto)
grub> configfile (YYY)/grub/grub.cfg

Where (XXX) is the encrypted partition and (YYY) is the boot partition with
the grub config, these can be found by doing `ls` command.
Attachment: file
L
L
Ludovic Courtès wrote on 25 May 2024 02:40
control message for bug #70826
(address . control@debbugs.gnu.org)
87le3y2psc.fsf@gnu.org
severity 70826 important
quit
L
L
Ludovic Courtès wrote on 25 May 2024 02:47
Re: bug#70826: luks-device-mapping-with-options breaks bootloader
(name . Tadhg McDonald-Jensen)(address . tadhgmister@gmail.com)(address . 70826@debbugs.gnu.org)
87ikz22pgo.fsf@gnu.org
Hi,

Tadhg McDonald-Jensen <tadhgmister@gmail.com> skribis:

Toggle quote (8 lines)
> using the `luks-device-mapping-with-options` mapped device type defined in
> (gnu system mapped-devices) causes grub or other bootloaders to not
> properly attempt to mount the encrypted drive. This is caused by the
> commit 39a9404 which identifies luks mapped devices by checking if the type
> is equal to `luks-device-mapping`, so by using a different routine that is
> a proxy to that one it doesn't forward it to grub in the
> store-crypto-devices list.

Ouch, indeed. The immediate fix is:
Toggle diff (21 lines)
diff --git a/gnu/system.scm b/gnu/system.scm
index c76f4d7c502..bb851b1b75f 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -667,10 +667,13 @@ (define (operating-system-boot-mapped-devices os)
(define operating-system-bootloader-crypto-devices
(mlambdaq (os) ;to avoid duplicated output
"Return the sources of the LUKS mapped devices specified by UUID."
+ (define (luks-device? m)
+ (memq (mapped-device-type m)
+ (list luks-device-mapping-with-options
+ luks-device-mapping)))
+
;; XXX: Device ordering is important, we trust the returned one.
- (let* ((luks-devices (filter (lambda (m)
- (eq? luks-device-mapping
- (mapped-device-type m)))
+ (let* ((luks-devices (filter luks-device?
(operating-system-boot-mapped-devices os)))
(uuid-crypto-devices non-uuid-crypto-devices
(partition (compose uuid? mapped-device-source)
Not ideal, but it fixes the problem.

I’ll go ahead with this patch if there are no objections.

Thanks!

Ludo’.
T
T
Tadhg McDonald-Jensen wrote on 25 May 2024 07:30
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 70826@debbugs.gnu.org)
ecfd1524-9d20-491c-b2be-8b7122c28d71@gmail.com
That unfortunately doesn't fix the problem,
`luks-device-mapping-with-options` is a routine that returns the
`mapped-device-kind` so it won't check by equality.

A possible solution is to check whether the `mapped-device-kind-close`
routines are the same as these are shared.


Toggle diff (79 lines)
diff --git a/gnu/system.scm b/gnu/system.scm
index cb6e719ca6..b564bf3788 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -661,10 +661,12 @@ (define (operating-system-boot-mapped-devices os)
(define operating-system-bootloader-crypto-devices
(mlambdaq (os) ;to avoid duplicated output
"Return the sources of the LUKS mapped devices specified by UUID."
+ (define (luks-device? m)
+ (eq? (mapped-device-kind-close (mapped-device-type m))
+ (mapped-device-kind-close luks-device-mapping)))
+
;; XXX: Device ordering is important, we trust the returned one.
- (let* ((luks-devices (filter (lambda (m)
- (eq? luks-device-mapping
- (mapped-device-type m)))
+ (let* ((luks-devices (filter luks-device?
(operating-system-boot-mapped-devices
os)))
(uuid-crypto-devices non-uuid-crypto-devices
(partition (compose uuid?
mapped-device-source)



(I apologize if my email client is adding line wraps to the diffs, I
will look into it after sending this)

I tried to implement this initially but it didn't work on my previous
attempt so I abandoned trying to submit a patch, but this version does
do the trick even if it seems inelegant.

On 2024-05-25 5:47 a.m., Ludovic Courtès wrote:
> Hi,
>
> Tadhg McDonald-Jensen <tadhgmister@gmail.com> skribis:
>
>> using the `luks-device-mapping-with-options` mapped device type defined in
>> (gnu system mapped-devices) causes grub or other bootloaders to not
>> properly attempt to mount the encrypted drive. This is caused by the
>> commit 39a9404 which identifies luks mapped devices by checking if the type
>> is equal to `luks-device-mapping`, so by using a different routine that is
>> a proxy to that one it doesn't forward it to grub in the
>> store-crypto-devices list.
>
> Ouch, indeed. The immediate fix is:
>
>
> diff --git a/gnu/system.scm b/gnu/system.scm
> index c76f4d7c502..bb851b1b75f 100644
> --- a/gnu/system.scm
> +++ b/gnu/system.scm
> @@ -667,10 +667,13 @@ (define (operating-system-boot-mapped-devices os)
> (define operating-system-bootloader-crypto-devices
> (mlambdaq (os) ;to avoid duplicated output
> "Return the sources of the LUKS mapped devices specified by UUID."
> + (define (luks-device? m)
> + (memq (mapped-device-type m)
> + (list luks-device-mapping-with-options
> + luks-device-mapping)))
> +
> ;; XXX: Device ordering is important, we trust the returned one.
> - (let* ((luks-devices (filter (lambda (m)
> - (eq? luks-device-mapping
> - (mapped-device-type m)))
> + (let* ((luks-devices (filter luks-device?
> (operating-system-boot-mapped-devices os)))
> (uuid-crypto-devices non-uuid-crypto-devices
> (partition (compose uuid? mapped-device-source)
>
>
>
> Not ideal, but it fixes the problem.
>
> I’ll go ahead with this patch if there are no objections.
>
> Thanks!
>
> Ludo’.
T
T
Tomas Volf wrote on 23 Jul 2024 11:19
(name . Tadhg McDonald-Jensen)(address . tadhgmister@gmail.com)
Zp_0RcfVu1bbXDoH@ws
On 2024-05-25 10:30:49 -0400, Tadhg McDonald-Jensen wrote:
Toggle quote (7 lines)
> That unfortunately doesn't fix the problem,
> `luks-device-mapping-with-options` is a routine that returns the
> `mapped-device-kind` so it won't check by equality.
>
> A possible solution is to check whether the `mapped-device-kind-close`
> routines are the same as these are shared.

What I find interesting is that I too am using luks-device-mapping-with-options
and my system boots just fine. So I wonder what the difference is. Could you
share your system configuration please? Or at least the relevant parts (I
assume at least bootloader, file-systems and mapped-devices fields)?

I would like to properly understand the problem here and why it works for me.

Thanks,
Tomas Volf

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEt4NJs4wUfTYpiGikL7/ufbZ/wakFAmaf9EUACgkQL7/ufbZ/
wan9vBAAnqC/KJO7HlcrJDLYAKJR8aVh9U2DywISrzC9cdqmmzDg6xwkTDFPDnKG
cYf+DF4T4dAn+TdlrF4KDWIoGqMLmMP/0l5fSaxPf4FdnDjY3BBr8As/qz1pxyKw
QUU4AcbedZCzXYT2V/pzy8LN6RpfNO6w9ojbvNFDmy03V83RxcCVDWQ5qLaV4ws1
EZ2irVcHkULJSfIslXGjPVmLIcihXHpIwv+zDCTE5IuM763RR02aJhqMWRONigEc
ysLfM8sVozp494MBpsSHxc3aIh1ZtjfxzQRTgYRBnmAIqbFZVwzkwP04iOsxv7CX
8/mi9yczq7RqsVFRK6QIgfepYfndJeA8ycCJEU7kjwZGgw3GdONa5uE1FeJmd6MV
OPqMb/Ttx7V5KgIo7k/1oGBR8afwaaDgW7ga/Oemdqim0lzKf6hpmrFlrfoAcsBw
MviLlDHA0vCVCybHehE2tGYs7QMMW/PNdWgUURXUrWGdxkMlJua2yJlLHBlvLMx2
G2ROY4mP8geARRD8yLOXskCh5aHSuC5T7yxP0Honno5P3aRDdW4iMTbwtSp1t+lM
BFaOesFcOF8VzyAUBKSsnlhiEdfcbjW/DIWeKRsuPevJ5iZyl/PiIlIxQQHsexHU
nbnwL3bix7M3q1ztt+OfrkbQThPz5cf69XrYi5fSwD9gKS4P35g=
=5ISi
-----END PGP SIGNATURE-----


T
T
Tadhg McDonald-Jensen wrote on 11 Aug 2024 15:33
(name . Tomas Volf)(address . ~@wolfsden.cz)
44aec6b7-dcba-4598-c984-068333cc696b@gmail.com
I have attached a config I just did `sudo guix system reconfigure`
and confirmed it was missing the `insmod luks` in /boot/grub/grub.cfg

Sorry for the delay,
Tadhg McD-J

On 2024-07-23 2:19 p.m., Tomas Volf wrote:
Toggle quote (21 lines)
> On 2024-05-25 10:30:49 -0400, Tadhg McDonald-Jensen wrote:
>> That unfortunately doesn't fix the problem,
>> `luks-device-mapping-with-options` is a routine that returns the
>> `mapped-device-kind` so it won't check by equality.
>>
>> A possible solution is to check whether the `mapped-device-kind-close`
>> routines are the same as these are shared.
>
> What I find interesting is that I too am using luks-device-mapping-with-options
> and my system boots just fine. So I wonder what the difference is. Could you
> share your system configuration please? Or at least the relevant parts (I
> assume at least bootloader, file-systems and mapped-devices fields)?
>
> I would like to properly understand the problem here and why it works for me.
>
> Thanks,
> Tomas Volf
>
> --
> There are only two hard things in Computer Science:
> cache invalidation, naming things and off-by-one errors.
Attachment: os.tmp.scm
T
T
Tadhg McDonald-Jensen wrote on 11 Aug 2024 16:19
(name . Tomas Volf)(address . ~@wolfsden.cz)
CAP5DvDh0pt=SdnW8ptHbKnjKHCY586Mis7chktQ8R3k-BH1o1w@mail.gmail.com
In case it is relevant my disk is using GPT partition table with this
layout:

$ lsblk --output="NAME,MAJ:MIN,TYPE,MOUNTPOINTS,UUID"
NAME MAJ:MIN TYPE MOUNTPOINTS UUID
nvme0n1 259:0 disk
├─nvme0n1p1 259:1 part /boot 5190-E840
└─nvme0n1p2 259:2 part c0010d06-0bd1-4ae2-93e6-f2f89a3a670b
└─cryptroot 253:0 crypt /gnu/store
/

Only the main partition is encrypted with LUKS and grub is located on
its own partition not in the in between space in an MBR drive.

It is grub that is being responsible for decrypting the partition
not my UEFI decrypting the whole drive.

Tadhg

On Sun, Aug 11, 2024 at 6:33 PM Tadhg McDonald-Jensen <tadhgmister@gmail.com>
wrote:

Toggle quote (32 lines)
> I have attached a config I just did `sudo guix system reconfigure`
> and confirmed it was missing the `insmod luks` in /boot/grub/grub.cfg
>
> Sorry for the delay,
> Tadhg McD-J
>
> On 2024-07-23 2:19 p.m., Tomas Volf wrote:
> > On 2024-05-25 10:30:49 -0400, Tadhg McDonald-Jensen wrote:
> >> That unfortunately doesn't fix the problem,
> >> `luks-device-mapping-with-options` is a routine that returns the
> >> `mapped-device-kind` so it won't check by equality.
> >>
> >> A possible solution is to check whether the `mapped-device-kind-close`
> >> routines are the same as these are shared.
> >
> > What I find interesting is that I too am using
> luks-device-mapping-with-options
> > and my system boots just fine. So I wonder what the difference is.
> Could you
> > share your system configuration please? Or at least the relevant parts
> (I
> > assume at least bootloader, file-systems and mapped-devices fields)?
> >
> > I would like to properly understand the problem here and why it works
> for me.
> >
> > Thanks,
> > Tomas Volf
> >
> > --
> > There are only two hard things in Computer Science:
> > cache invalidation, naming things and off-by-one errors.
Attachment: file
4
4
45mg wrote on 4 Apr 12:44 -0700
[PATCH] system: Allow distinguishing <mapped-device-type>s.
(address . 70826@debbugs.gnu.org)
4e7d4ef33cc34ac5cb73a525d294d4b7f44f6989.1743795895.git.45mg.writes@gmail.com
We use <mapped-device-type> records to represent the different types of
mapped devices (LUKS, RAID, LVM). When variables are defined for these
records, we can distinguish them with eq?; when they are created by
procedures, like luks-device-mapping-with-options, this does not work.
Therefore, add a 'name' field to <mapped-device-type> to distinguish
them.

* gnu/system/mapped-devices.scm (<mapped-device-type>): Add name field.
(luks-device-mapping, raid-device-mapping, lvm-device-mapping):
Initialize it with appropriate values for each of these types.
* gnu/system.scm (operating-system-bootloader-crypto-devices): Use it to
identify LUKS mapped devices.

Change-Id: I4c85824f74316f07239374d9df6c007dd47a9d0c
---

I've tested this on my system; in conjunction with [1], I can finally mount my
LUKS volume with the no_read_workqueue and no_write_workqueue flags.

[1] [bug#77499] [PATCH] mapped-devices/luks: Support extra options.


gnu/system.scm | 6 ++++--
gnu/system/mapped-devices.scm | 6 ++++++
2 files changed, 10 insertions(+), 2 deletions(-)

Toggle diff (80 lines)
diff --git a/gnu/system.scm b/gnu/system.scm
index 0d98e5a036..87247f06ee 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -16,6 +16,7 @@
;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
;;; Copyright © 2024 Nicolas Graves <ngraves@ngraves.fr>
+;;; Copyright © 2025 45mg <45mg.writes@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -676,8 +677,9 @@ (define operating-system-bootloader-crypto-devices
"Return the sources of the LUKS mapped devices specified by UUID."
;; XXX: Device ordering is important, we trust the returned one.
(let* ((luks-devices (filter (lambda (m)
- (eq? luks-device-mapping
- (mapped-device-type m)))
+ (eq? (mapped-device-kind-name
+ (mapped-device-type m))
+ 'luks))
(operating-system-boot-mapped-devices os)))
(uuid-crypto-devices non-uuid-crypto-devices
(partition (compose uuid? mapped-device-source)
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 667a495570..50626b8df9 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -3,6 +3,7 @@
;;; Copyright © 2016 Andreas Enge <andreas@enge.fr>
;;; Copyright © 2017, 2018 Mark H Weaver <mhw@netris.org>
;;; Copyright © 2024 Tomas Volf <~@wolfsden.cz>
+;;; Copyright © 2025 45mg <45mg.writes@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -55,6 +56,7 @@ (define-module (gnu system mapped-devices)
mapped-device-kind
mapped-device-kind?
+ mapped-device-kind-name
mapped-device-kind-open
mapped-device-kind-close
mapped-device-kind-modules
@@ -110,6 +112,7 @@ (define-deprecated (mapped-device-target md)
(define-record-type* <mapped-device-type> mapped-device-kind
make-mapped-device-kind
mapped-device-kind?
+ (name mapped-device-kind-name)
(open mapped-device-kind-open) ;source target -> gexp
(close mapped-device-kind-close ;source target -> gexp
(default (const #~(const #f))))
@@ -283,6 +286,7 @@ (define* (check-luks-device md #:key
(define luks-device-mapping
;; The type of LUKS mapped devices.
(mapped-device-kind
+ (name 'luks)
(open open-luks-device)
(close close-luks-device)
(check check-luks-device)
@@ -338,6 +342,7 @@ (define (close-raid-device sources targets)
(define raid-device-mapping
;; The type of RAID mapped devices.
(mapped-device-kind
+ (name 'raid)
(open open-raid-device)
(close close-raid-device)))
@@ -358,6 +363,7 @@ (define (close-lvm-device source targets)
(define lvm-device-mapping
(mapped-device-kind
+ (name 'lvm)
(open open-lvm-device)
(close close-lvm-device)
(modules '((srfi srfi-1)))))

base-commit: 0b754ceeded322e8079130e6793b0c68356967cf
--
2.49.0
L
L
Ludovic Courtès wrote 7 days ago
(name . 45mg)(address . 45mg.writes@gmail.com)
87jz7rsu49.fsf@gnu.org
Hi,

45mg <45mg.writes@gmail.com> skribis:

Toggle quote (23 lines)
> We use <mapped-device-type> records to represent the different types of
> mapped devices (LUKS, RAID, LVM). When variables are defined for these
> records, we can distinguish them with eq?; when they are created by
> procedures, like luks-device-mapping-with-options, this does not work.
> Therefore, add a 'name' field to <mapped-device-type> to distinguish
> them.
>
> * gnu/system/mapped-devices.scm (<mapped-device-type>): Add name field.
> (luks-device-mapping, raid-device-mapping, lvm-device-mapping):
> Initialize it with appropriate values for each of these types.
> * gnu/system.scm (operating-system-bootloader-crypto-devices): Use it to
> identify LUKS mapped devices.
>
> Change-Id: I4c85824f74316f07239374d9df6c007dd47a9d0c
> ---
>
> I've tested this on my system; in conjunction with [1], I can finally mount my
> LUKS volume with the no_read_workqueue and no_write_workqueue flags.
>
> [1] [bug#77499] [PATCH] mapped-devices/luks: Support extra options.
> https://issues.guix.gnu.org/77499
> https://yhetil.org/guix/fb637872bd14abe305d810b9d32e0db290b26dd6.1743702237.git.45mg.writes@gmail.com/

You can add a “Fixes” line for the bug it fixes.

Toggle quote (7 lines)
> (let* ((luks-devices (filter (lambda (m)
> - (eq? luks-device-mapping
> - (mapped-device-type m)))
> + (eq? (mapped-device-kind-name
> + (mapped-device-type m))
> + 'luks))

[...]

Toggle quote (5 lines)
> (define-record-type* <mapped-device-type> mapped-device-kind
> make-mapped-device-kind
> mapped-device-kind?
> + (name mapped-device-kind-name)

As a rule of thumb, I think comparing by identity (as was done before)
is more robust and cleaner: that avoids the whole problem of this
secondary name space where name clashes may occur involuntarily.

But this problem the patch fixes was introduced by
‘luks-device-mapping-with-options’ I believe, which returns a new device
type.

If we take a step back, I wonder if a better solution would not be to
add an ‘arguments’ field to <mapped-device>, following the same pattern
as <package>.

Here’s a preliminary patch to illustrate that:
Toggle diff (64 lines)
diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
index 72da8e55d3..17c2e6f6bf 100644
--- a/gnu/system/linux-initrd.scm
+++ b/gnu/system/linux-initrd.scm
@@ -229,7 +229,8 @@ (define* (raw-initrd file-systems
(targets (mapped-device-targets md))
(type (mapped-device-type md))
(open (mapped-device-kind-open type)))
- (open source targets)))
+ (apply open source targets
+ (mapped-device-arguments md))))
mapped-devices))
(define file-system-scan-commands
diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
index 667a495570..29d0dc95cf 100644
--- a/gnu/system/mapped-devices.scm
+++ b/gnu/system/mapped-devices.scm
@@ -83,6 +83,8 @@ (define-record-type* <mapped-device> %mapped-device
(source mapped-device-source) ;string | list of strings
(targets mapped-device-targets) ;list of strings
(type mapped-device-type) ;<mapped-device-kind>
+ (arguments mapped-device-arguments ;list passed to open/close/check
+ (default '()))
(location mapped-device-location
(default (current-source-location)) (innate)))
@@ -128,13 +130,16 @@ (define device-mapping-service-type
'device-mapping
(match-lambda
(($ <mapped-device> source targets
- ($ <mapped-device-type> open close modules))
+ ($ <mapped-device-type> open close modules)
+ arguments)
(shepherd-service
(provision (list (symbol-append 'device-mapping- (string->symbol (string-join targets "-")))))
(requirement '(udev))
(documentation "Map a device node using Linux's device mapper.")
- (start #~(lambda () #$(open source targets)))
- (stop #~(lambda _ (not #$(close source targets))))
+ (start #~(lambda ()
+ #$(apply open source targets arguments)))
+ (stop #~(lambda _
+ (not #$(apply close source targets arguments))))
(modules (append %default-modules modules))
(respawn? #f))))
(description "Map a device node using Linux's device mapper.")))
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 23eb215561..8a56f1cc63 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -680,9 +680,10 @@ (define (check-mapped-devices os)
(mapped-device-type md))))
;; We expect CHECK to raise an exception with a detailed
;; '&message' if something goes wrong.
- (check md
+ (apply check md
#:needed-for-boot? (needed-for-boot? md)
- #:initrd-modules initrd-modules)))
+ #:initrd-modules initrd-modules
+ (mapped-device-arguments md))))
(operating-system-mapped-devices os)))
(define (check-initrd-modules os)
WDYT?
Ludo’.
T
T
Tomas Volf wrote 7 days ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ecxz65ry.fsf@wolfsden.cz
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (45 lines)
> [..]
>> (define-record-type* <mapped-device-type> mapped-device-kind
>> make-mapped-device-kind
>> mapped-device-kind?
>> + (name mapped-device-kind-name)
>
> As a rule of thumb, I think comparing by identity (as was done before)
> is more robust and cleaner: that avoids the whole problem of this
> secondary name space where name clashes may occur involuntarily.
>
> But this problem the patch fixes was introduced by
> ‘luks-device-mapping-with-options’ I believe, which returns a new device
> type.
>
> If we take a step back, I wonder if a better solution would not be to
> add an ‘arguments’ field to <mapped-device>, following the same pattern
> as <package>.
>
> Here’s a preliminary patch to illustrate that:
>
> diff --git a/gnu/system/linux-initrd.scm b/gnu/system/linux-initrd.scm
> index 72da8e55d3..17c2e6f6bf 100644
> --- a/gnu/system/linux-initrd.scm
> +++ b/gnu/system/linux-initrd.scm
> @@ -229,7 +229,8 @@ (define* (raw-initrd file-systems
> (targets (mapped-device-targets md))
> (type (mapped-device-type md))
> (open (mapped-device-kind-open type)))
> - (open source targets)))
> + (apply open source targets
> + (mapped-device-arguments md))))
> mapped-devices))
>
> (define file-system-scan-commands
> diff --git a/gnu/system/mapped-devices.scm b/gnu/system/mapped-devices.scm
> index 667a495570..29d0dc95cf 100644
> --- a/gnu/system/mapped-devices.scm
> +++ b/gnu/system/mapped-devices.scm
> @@ -83,6 +83,8 @@ (define-record-type* <mapped-device> %mapped-device
> (source mapped-device-source) ;string | list of strings
> (targets mapped-device-targets) ;list of strings
> (type mapped-device-type) ;<mapped-device-kind>
> + (arguments mapped-device-arguments ;list passed to open/close/check
> + (default '()))

I do not think it is a good idea to expect the same identical list for
all of the operations, so we would need arguments-open, arguments-close
and arguments-check fields. Other that that, this would work, sure.

Toggle quote (44 lines)
> (location mapped-device-location
> (default (current-source-location)) (innate)))
>
> @@ -128,13 +130,16 @@ (define device-mapping-service-type
> 'device-mapping
> (match-lambda
> (($ <mapped-device> source targets
> - ($ <mapped-device-type> open close modules))
> + ($ <mapped-device-type> open close modules)
> + arguments)
> (shepherd-service
> (provision (list (symbol-append 'device-mapping- (string->symbol (string-join targets "-")))))
> (requirement '(udev))
> (documentation "Map a device node using Linux's device mapper.")
> - (start #~(lambda () #$(open source targets)))
> - (stop #~(lambda _ (not #$(close source targets))))
> + (start #~(lambda ()
> + #$(apply open source targets arguments)))
> + (stop #~(lambda _
> + (not #$(apply close source targets arguments))))
> (modules (append %default-modules modules))
> (respawn? #f))))
> (description "Map a device node using Linux's device mapper.")))
> diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
> index 23eb215561..8a56f1cc63 100644
> --- a/guix/scripts/system.scm
> +++ b/guix/scripts/system.scm
> @@ -680,9 +680,10 @@ (define (check-mapped-devices os)
> (mapped-device-type md))))
> ;; We expect CHECK to raise an exception with a detailed
> ;; '&message' if something goes wrong.
> - (check md
> + (apply check md
> #:needed-for-boot? (needed-for-boot? md)
> - #:initrd-modules initrd-modules)))
> + #:initrd-modules initrd-modules
> + (mapped-device-arguments md))))
> (operating-system-mapped-devices os)))
>
> (define (check-initrd-modules os)
>
>
> WDYT?

Only thing I wonder about whether we should be apply-ing the list, or
just passing it as a final arguments. That would be a change in the
calling convention, but all procedures could just be adjusted to take
the extra argument.

One question would be how to handle if user passes the extra arguments
to a procedure that is not prepared to handle them. In your approach
(apply) it would be a error due to wrong number of arguments, I am not
sure whether that is ideal. Maybe a warning would be better? Not sure.
Opinions?

Tomas

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.
L
L
Ludovic Courtès wrote 6 days ago
(name . Tomas Volf)(address . ~@wolfsden.cz)
87jz7rp0ft.fsf@gnu.org
Hello,

Tomas Volf <~@wolfsden.cz> skribis:

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (12 lines)
>> +++ b/gnu/system/mapped-devices.scm
>> @@ -83,6 +83,8 @@ (define-record-type* <mapped-device> %mapped-device
>> (source mapped-device-source) ;string | list of strings
>> (targets mapped-device-targets) ;list of strings
>> (type mapped-device-type) ;<mapped-device-kind>
>> + (arguments mapped-device-arguments ;list passed to open/close/check
>> + (default '()))
>
> I do not think it is a good idea to expect the same identical list for
> all of the operations, so we would need arguments-open, arguments-close
> and arguments-check fields. Other that that, this would work, sure.

Yeah I wondered about that but I thought that in the worst case the
operations could just ignore those arguments; from the user’s viewpoint,
that would keep the interface simple.

Toggle quote (5 lines)
> Only thing I wonder about whether we should be apply-ing the list, or
> just passing it as a final arguments. That would be a change in the
> calling convention, but all procedures could just be adjusted to take
> the extra argument.

It’s a backward-compatible change in the calling convention (nothing
changes for existing open/close/check methods that accept no keyword
arguments).

Toggle quote (6 lines)
> One question would be how to handle if user passes the extra arguments
> to a procedure that is not prepared to handle them. In your approach
> (apply) it would be a error due to wrong number of arguments, I am not
> sure whether that is ideal. Maybe a warning would be better? Not sure.
> Opinions?

Yes, you’d get a wrong-type-arg error, similar to what you have if you
pass #:random-number 42 in the ‘arguments’ field of a package.

It’s not ideal in terms of error reporting, but maybe good enough.

Another option would be for instance to always pass a key/value pair;
open/close/check methods would have to look up in that list for relevant
keys and could provide better error reporting, but the downside is extra
boilerplate in those methods.

Ludo’.
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 70826@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 70826
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch