| Message ID | 20220514060532.1011-1-maxim.cournoyer@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#55407] system: Improve warning when using LUKS mapped devices without UUIDs. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
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’.
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
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’.
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."