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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55407 in the body.
You can then email your comments to 55407 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH] system: Improve warning when using LUKS mapped devices
 without UUIDs.
Date: Sat, 14 May 2022 02:05:32 -0400

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

Message #8 received at 55407 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 55407 <at> debbugs.gnu.org
Subject: Re: bug#55407: [PATCH] system: Improve warning when using LUKS
 mapped devices without UUIDs.
Date: Wed, 18 May 2022 22:44:57 +0200

Hi!

Maxim Cournoyer <maxim.cournoyer <at> 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’.

Message #13 received at 55407-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 55407-done <at> debbugs.gnu.org
Subject: Re: bug#55407: [PATCH] system: Improve warning when using LUKS
 mapped devices without UUIDs.
Date: Sat, 21 May 2022 00:17:25 -0400

Hi,

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

> Hi!
>
> Maxim Cournoyer <maxim.cournoyer <at> 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

Message #16 received at 55407-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 55407-done <at> debbugs.gnu.org
Subject: Re: bug#55407: [PATCH] system: Improve warning when using LUKS
 mapped devices without UUIDs.
Date: Sat, 21 May 2022 18:46:03 +0200

Hi!

Maxim Cournoyer <maxim.cournoyer <at> 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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.