Package: guix-patches;
Reported by: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Date: Sat, 14 May 2022 06:06:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.