Subject: bug#70826: [PATCH] system: Allow distinguishing <mapped-device-type>s.

From: Tomas Volf <[email-masked]>
Date: Thu, 10 Apr 2025 23:55:13 +0200

Ludovic Courtès <[email-masked]> writes:

> [..]
>>  (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.

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