[bug#55407] [PATCH] system: Improve warning when using LUKS mapped devices without UUIDs.

[bug#55407] [PATCH] system: Improve warning when using LUKS mapped devices without UUIDs.

[Maxim Cournoyer]

This corrects two problems with the previous mapped devices warning:

1. It wasn't clear how to correct the situation.
2. The output would be repeated twice, as the procedure is called
twice during a system reconfigure.

* gnu/system.scm (operating-system-bootloader-crypto-devices): Memoize
procedure.  Produce a single message for the combined problematic devices.
Add a hint to help users fix the warning.
---
 gnu/system.scm | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/gnu/system.scm b/gnu/system.scm
index c3810cbeeb..b090eeae01 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -33,6 +33,7 @@
 (define-module (gnu system)
   #:use-module (guix inferior)
   #:use-module (guix store)
+  #:use-module (guix memoization)
   #:use-module (guix monads)
   #:use-module (guix gexp)
   #:use-module (guix records)
@@ -78,7 +79,9 @@ (define-module (gnu system)
   #:use-module (gnu system uuid)
   #:use-module (gnu system file-systems)
   #:use-module (gnu system mapped-devices)
+  #:use-module (ice-9 format)
   #:use-module (ice-9 match)
+  #:use-module (ice-9 receive)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
@@ -600,25 +603,26 @@ (define (operating-system-boot-mapped-devices os)
                (any file-system-needed-for-boot? users)))
            devices)))
 
-(define (operating-system-bootloader-crypto-devices os)
-  "Return the subset of mapped devices that the bootloader must open.
-Only devices specified by uuid are supported."
-  (define (valid-crypto-device? dev)
-    (or (uuid? dev)
-        (begin
-          (warning (G_ "\
-mapped-device '~a' may not be mounted by the bootloader.~%")
-                   dev)
-          #f)))
-  (filter-map (match-lambda
-                ((and (= mapped-device-type type)
-                      (= mapped-device-source source))
-                 (and (eq? luks-device-mapping type)
-                      (valid-crypto-device? source)
-                      source))
-                (_ #f))
-              ;; XXX: Ordering is important, we trust the returned one.
-              (operating-system-boot-mapped-devices os)))
+(define operating-system-bootloader-crypto-devices
+  (mlambda (os)                         ;to avoid duplicated output
+    "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)))
+                                (operating-system-boot-mapped-devices os))))
+      (receive (uuid-crypto-devices non-uuid-crypto-devices)
+          (partition (compose uuid? mapped-device-source) luks-devices)
+        (when (not (null? non-uuid-crypto-devices))
+          (warning (N_ "\
+the following mapped device may not be mounted by the bootloader: ~s
+hint: specify the mapped device source via its LUKS UUID.~%"
+                       "\
+the following mapped devices may not be mounted by the bootloader: ~s
+hint: specify the mapped device sources via their LUKS UUID.~%"
+                       (length non-uuid-crypto-devices))
+                   (map mapped-device-source non-uuid-crypto-devices)))
+        (map mapped-device-source uuid-crypto-devices)))))
 
 (define (device-mapping-services os)
   "Return the list of device-mapping services for OS as a list."
-- 
2.36.0

[bug#55407] [PATCH] system: Improve warning when using LUKS mapped devices without UUIDs.

[Ludovic Courtès]

Hi!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> This corrects two problems with the previous mapped devices warning:
>
> 1. It wasn't clear how to correct the situation.
> 2. The output would be repeated twice, as the procedure is called
> twice during a system reconfigure.
>
> * gnu/system.scm (operating-system-bootloader-crypto-devices): Memoize
> procedure.  Produce a single message for the combined problematic devices.
> Add a hint to help users fix the warning.

[...]

> +(define operating-system-bootloader-crypto-devices
> +  (mlambda (os)                         ;to avoid duplicated output

Should be ‘mlambdaq’ so that OS is compared with ‘eq?’, which is cheaper
and better corresponds to what we want to achieve here.

> +      (receive (uuid-crypto-devices non-uuid-crypto-devices)
> +          (partition (compose uuid? mapped-device-source) luks-devices)

I suggest using ‘let’ from (srfi srfi-71) for consistency.

> +        (when (not (null? non-uuid-crypto-devices))
> +          (warning (N_ "\
> +the following mapped device may not be mounted by the bootloader: ~s
> +hint: specify the mapped device source via its LUKS UUID.~%"
> +                       "\
> +the following mapped devices may not be mounted by the bootloader: ~s
> +hint: specify the mapped device sources via their LUKS UUID.~%"
> +                       (length non-uuid-crypto-devices))
> +                   (map mapped-device-source non-uuid-crypto-devices)))

By convention, warnings should fit on a single line and not be full
sentences.

If we emit one warning per mapped device, we can report the source
location using:

  (warning (mapped-device-location dev) (G_ "mapped device …"))

Given that we have location info, it might be better to report one
warning per device?

Last, hints should be reported either with ‘display-hint’ or with a
‘&fix-hint’ exception.  The hint itself can be one or two paragraphs
using Texinfo markup.

All this should ensure consistent diagnostic reporting.

I hope this makes sense!

Thanks,
Ludo’.

bug#55407: [PATCH] system: Improve warning when using LUKS mapped devices without UUIDs.

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> This corrects two problems with the previous mapped devices warning:
>>
>> 1. It wasn't clear how to correct the situation.
>> 2. The output would be repeated twice, as the procedure is called
>> twice during a system reconfigure.
>>
>> * gnu/system.scm (operating-system-bootloader-crypto-devices): Memoize
>> procedure.  Produce a single message for the combined problematic devices.
>> Add a hint to help users fix the warning.
>
> [...]
>
>> +(define operating-system-bootloader-crypto-devices
>> +  (mlambda (os)                         ;to avoid duplicated output
>
> Should be ‘mlambdaq’ so that OS is compared with ‘eq?’, which is cheaper
> and better corresponds to what we want to achieve here.

Done.

>> +      (receive (uuid-crypto-devices non-uuid-crypto-devices)
>> +          (partition (compose uuid? mapped-device-source) luks-devices)
>
> I suggest using ‘let’ from (srfi srfi-71) for consistency.

Done.

>> +        (when (not (null? non-uuid-crypto-devices))
>> +          (warning (N_ "\
>> +the following mapped device may not be mounted by the bootloader: ~s
>> +hint: specify the mapped device source via its LUKS UUID.~%"
>> +                       "\
>> +the following mapped devices may not be mounted by the bootloader: ~s
>> +hint: specify the mapped device sources via their LUKS UUID.~%"
>> +                       (length non-uuid-crypto-devices))
>> +                   (map mapped-device-source non-uuid-crypto-devices)))
>
> By convention, warnings should fit on a single line and not be full
> sentences.

This is a Guix-specific convention, right?  I couldn't find a reference
to it in the GNU Standards (info standards) document.  I'd be more of
the thinking that warnings directed at *users* should be as human
readable as possible; the motivation for my fix was because that for
more than a year, I read that warning without having clue about what it
really meant and had to review the source to get the answer.

> If we emit one warning per mapped device, we can report the source
> location using:
>
>   (warning (mapped-device-location dev) (G_ "mapped device …"))
>
> Given that we have location info, it might be better to report one
> warning per device?

Done!

> Last, hints should be reported either with ‘display-hint’ or with a
> ‘&fix-hint’ exception.  The hint itself can be one or two paragraphs
> using Texinfo markup.
>
> All this should ensure consistent diagnostic reporting.
>
> I hope this makes sense!

It does.  I didn't know about warning accepting the location like that,
and I thought hints were only usable via conditions, so I've learned a
couple new things here.

With the following changes:

--8<---------------cut here---------------start------------->8---
modified   gnu/system.scm
@@ -43,6 +43,7 @@ (define-module (gnu system)
   #:use-module ((guix utils) #:select (substitute-keyword-arguments))
   #:use-module (guix i18n)
   #:use-module (guix diagnostics)
+  #:use-module (guix ui)
   #:use-module (gnu packages admin)
   #:use-module (gnu packages base)
   #:use-module (gnu packages bash)
@@ -81,11 +82,11 @@ (define-module (gnu system)
   #:use-module (gnu system mapped-devices)
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
-  #:use-module (ice-9 receive)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
+  #:use-module (srfi srfi-71)
   #:use-module (rnrs bytevectors)
   #:export (operating-system
             operating-system?
@@ -604,25 +605,25 @@ (define (operating-system-boot-mapped-devices os)
            devices)))

 (define operating-system-bootloader-crypto-devices
-  (mlambda (os)                         ;to avoid duplicated output
+  (mlambdaq (os)                        ;to avoid duplicated output
     "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)))
-                                (operating-system-boot-mapped-devices os))))
-      (receive (uuid-crypto-devices non-uuid-crypto-devices)
-          (partition (compose uuid? mapped-device-source) luks-devices)
-        (when (not (null? non-uuid-crypto-devices))
-          (warning (N_ "\
-the following mapped device may not be mounted by the bootloader: ~s
-hint: specify the mapped device source via its LUKS UUID.~%"
-                       "\
-the following mapped devices may not be mounted by the bootloader: ~s
-hint: specify the mapped device sources via their LUKS UUID.~%"
-                       (length non-uuid-crypto-devices))
-                   (map mapped-device-source non-uuid-crypto-devices)))
-        (map mapped-device-source uuid-crypto-devices)))))
+    (let* ((luks-devices (filter (lambda (m)
+                                   (eq? luks-device-mapping
+                                        (mapped-device-type m)))
+                                 (operating-system-boot-mapped-devices os)))
+           (uuid-crypto-devices non-uuid-crypto-devices
+                                (partition (compose uuid? mapped-device-source)
+                                           luks-devices)))
+      (when (not (null? non-uuid-crypto-devices))
+        (for-each (lambda (dev)
+                    (warning
+                     (source-properties->location (mapped-device-location dev))
+                     (G_ "mapped device '~a' may be ignored by bootloader~%")
+                     (mapped-device-source dev)))
+                  non-uuid-crypto-devices)
+        (display-hint "Specify mapped device sources via their LUKS UUID."))
+      (map mapped-device-source uuid-crypto-devices))))

 (define (device-mapping-services os)
   "Return the list of device-mapping services for OS as a list."
--8<---------------cut here---------------end--------------->8---

It produced this output:

--8<---------------cut here---------------start------------->8---
/home/maxim/stow/guix/hurd.scm:109:8: warning: mapped device '/dev/sda2' may be ignored by bootloader
/home/maxim/stow/guix/hurd.scm:113:8: warning: mapped device '/dev/sdb2' may be ignored by bootloader
/home/maxim/stow/guix/hurd.scm:117:8: warning: mapped device '/dev/sdc2' may be ignored by bootloader
hint: Specify mapped device sources via their LUKS UUID.

updating checkout of 'file:///home/maxim/src/mcron'...
retrieved commit a233aab1b6770bece7538cda8381df824b7560b6
--8<---------------cut here---------------end--------------->8---

which compares favorably to before the change:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix system reconfigure ~/stow/guix/hurd.scm
guix system: warning: mapped-device '/dev/sda2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdb2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdc2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sda2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdb2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdc2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sda2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdb2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdc2' may not be mounted by the bootloader.
updating checkout of 'file:///home/maxim/src/mcron'...
retrieved commit a233aab1b6770bece7538cda8381df824b7560b6
guix system: warning: mapped-device '/dev/sda2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdb2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdc2' may not be mounted by the bootloader.
updating checkout of 'file:///home/maxim/src/mcron'...
retrieved commit a233aab1b6770bece7538cda8381df824b7560b6
substitute: updating substitutes from 'http://127.0.0.1:8181'... 100.0%

[...]

activating system...
guix system: warning: mapped-device '/dev/sda2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdb2' may not be mounted by the bootloader.
guix system: warning: mapped-device '/dev/sdc2' may not be mounted by the bootloader.
[...]
--8<---------------cut here---------------end--------------->8---

Pushed as 39a9404c99.

Thanks for the useful comments!

Maxim

[bug#55407] [PATCH] system: Improve warning when using LUKS mapped devices without UUIDs.

[Ludovic Courtès]

Hi!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>>> +          (warning (N_ "\
>>> +the following mapped device may not be mounted by the bootloader: ~s
>>> +hint: specify the mapped device source via its LUKS UUID.~%"
>>> +                       "\
>>> +the following mapped devices may not be mounted by the bootloader: ~s
>>> +hint: specify the mapped device sources via their LUKS UUID.~%"
>>> +                       (length non-uuid-crypto-devices))
>>> +                   (map mapped-device-source non-uuid-crypto-devices)))
>>
>> By convention, warnings should fit on a single line and not be full
>> sentences.
>
> This is a Guix-specific convention, right?  I couldn't find a reference
> to it in the GNU Standards (info standards) document.

It’s more or less the GNU convention (info "(standards) Errors").

The bit about hints is Guix-specific, but it’s the same idea: having a
consistent way to report diagnostics.

> I'd be more of the thinking that warnings directed at *users* should
> be as human readable as possible; the motivation for my fix was
> because that for more than a year, I read that warning without having
> clue about what it really meant and had to review the source to get
> the answer.

Yes, and I agree that’s a problem.  Hopefully hints help address that.

(The Elm compiler for instance is famous for having verbose diagnostics
*and* hints.  Perhaps something to look at and take inspiration from in
the future.)

> It produced this output:
>
> /home/maxim/stow/guix/hurd.scm:109:8: warning: mapped device '/dev/sda2' may be ignored by bootloader
> /home/maxim/stow/guix/hurd.scm:113:8: warning: mapped device '/dev/sdb2' may be ignored by bootloader
> /home/maxim/stow/guix/hurd.scm:117:8: warning: mapped device '/dev/sdc2' may be ignored by bootloader
> hint: Specify mapped device sources via their LUKS UUID.

Nice.  You could even add an @example block in the hint to illustrate
what that means.

> Pushed as 39a9404c99.

Thank you!

Ludo’.