diff mbox series

[bug#37305,v3] Allow booting from a Btrfs subvolume

Message ID 87lflc65ra.fsf@gmail.com
State Accepted
Headers show
Series [bug#37305,v3] Allow booting from a Btrfs subvolume | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job

Commit Message

Maxim Cournoyer May 28, 2020, 4:30 a.m. UTC
Hello,

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> To me, another consideration is familiarity with Btrfs for those who’ll
>> touch the code: to someone not familiar with it, the code may be viewed
>> as “read-only” because it says “btrfs”.  Whereas if it clearly states
>> that it’s just about prepending a directory name or similar, it’s easy
>> to reason about it.
>
> Agreed, this is where I was going to with my comment on ZFS.
> Maybe the "btrfs" part of the symbols can be left out to make it more
> general and understandable.

I've adapted with the naming suggested earlier by Ludovic.  Does the
patch below fit the bill?

Thanks,

Maxim

Comments

Pierre Neidhardt May 28, 2020, 8:26 a.m. UTC | #1
Looks good to me, but:

>  			  (btrfs-store-subvolume-file-name file-systems))))

I haven't looked at the context, but is this procedure really Btrfs-only?
Ludovic Courtès May 28, 2020, 12:30 p.m. UTC | #2
Hello!

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

> From ee23cc391cce7b8dcdcb5146d4b84a55881a5cb9 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Wed, 27 May 2020 22:44:28 -0400
> Subject: [PATCH] bootloader: grub: Rename the btrfs-subvolume-file-name
>  parameter.
>
> Following discussion in <https://issues.guix.gnu.org/37305>, it seems more
> appropriate to give the parameter a more generic name that better describes
> what it does.
>
> * gnu/bootloader/grub.scm (normalize-file): Rename the
> btrfs-subvolume-file-name parameter to store-directory-prefix.
> (eye-candy): Likewise.
> (grub-configuration-file): Likewise.
> * gnu/system.scm (operating-system-bootcfg): Adapt.

[...]

>  (define* (eye-candy config store-device store-mount-point
> -                    #:key btrfs-store-subvolume-file-name system port)
> +                    #:key store-directory-prefix system port)
>    "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
>  concerned with graphics mode, background images, colors, and all that.
>  STORE-DEVICE designates the device holding the store, and STORE-MOUNT-POINT is
>  its mount point; these are used to determine where the background image and
>  fonts must be searched for.  SYSTEM must be the target system string---e.g.,
> -\"x86_64-linux\".  BTRFS-STORE-SUBVOLUME-FILE-NAME is the file name of the
> -Btrfs subvolume, to be prepended to any store path, if any."
> +\"x86_64-linux\".  STORE-DIRECTORY-PREFIX is a directory prefix to prepend to
> +any store path."

s/path/file name/  :-)

Maybe you can have ‘store-directory-prefix’ default to "" and adjust
users accordingly.

Otherwise LGTM.

Thanks for taking the time to work on this patch!

Ludo’.
Maxim Cournoyer May 29, 2020, 9:14 p.m. UTC | #3
Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Looks good to me, but:
>
>>  			  (btrfs-store-subvolume-file-name file-systems))))
>
> I haven't looked at the context, but is this procedure really Btrfs-only?

Yes!  It takes a list of file-system objects, filter the Btrfs ones
using the "subvol" option and returns their subvolume file name, if the
store is mounted on such subvolume.
Maxim Cournoyer May 30, 2020, 2 a.m. UTC | #4
Hello,

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

[...]

>> +\"x86_64-linux\".  STORE-DIRECTORY-PREFIX is a directory prefix to prepend to
>> +any store path."
>
> s/path/file name/  :-)

Done!

> Maybe you can have ‘store-directory-prefix’ default to "" and adjust
> users accordingly.

I tried adapting it, but my test failed, because I had failed to adapt
one of the users.  I find that using #f to denote the absence of a
prefix more accurate than "" (after all, "" is a valid prefix) and
composes better (should we want to 'or' a couple of prefix seeking
procedures together).

> Otherwise LGTM.
>
> Thanks for taking the time to work on this patch!

No problem :-) Pushed as commit e7b86a0d88.

Maxim
Pierre Neidhardt May 30, 2020, 7:32 a.m. UTC | #5
Hooray!
Pierre Neidhardt May 30, 2020, 7:32 a.m. UTC | #6
Maybe this would warrant a blog post! :)
Maxim Cournoyer May 31, 2020, 2:44 a.m. UTC | #7
Hello Pierre!

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Maybe this would warrant a blog post! :)

Eh, I'm not sure.  It's kind of a basic thing that other systems already
support (through grub-mkconfig).  We've just caught up :-).

Maxim
Pierre Neidhardt May 31, 2020, 7:32 a.m. UTC | #8
Sure, but I don't know how popular Btrfs is, plus compression is a very
welcome addition to Guix (for all those SSD users with little space :p).

Granted, this is not very Guix specific...
diff mbox series

Patch

From ee23cc391cce7b8dcdcb5146d4b84a55881a5cb9 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Wed, 27 May 2020 22:44:28 -0400
Subject: [PATCH] bootloader: grub: Rename the btrfs-subvolume-file-name
 parameter.

Following discussion in <https://issues.guix.gnu.org/37305>, it seems more
appropriate to give the parameter a more generic name that better describes
what it does.

* gnu/bootloader/grub.scm (normalize-file): Rename the
btrfs-subvolume-file-name parameter to store-directory-prefix.
(eye-candy): Likewise.
(grub-configuration-file): Likewise.
* gnu/system.scm (operating-system-bootcfg): Adapt.
---
 gnu/bootloader/grub.scm | 46 ++++++++++++++++++++---------------------
 gnu/system.scm          |  2 +-
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index bb40c551a7..7b2fc18103 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -58,8 +58,8 @@ 
 ;;;
 ;;; Code:
 
-(define* (normalize-file file mount-point btrfs-subvolume-file-name)
-  "Strip MOUNT-POINT and prepend BTRFS-SUBVOLUME-FILE-NAME to FILE, a
+(define* (normalize-file file mount-point store-directory-prefix)
+  "Strip MOUNT-POINT and prepend STORE-DIRECTORY-PREFIX to FILE, a
 G-expression or other lowerable object denoting a file name."
 
   (define (strip-mount-point mount-point file)
@@ -72,12 +72,12 @@  G-expression or other lowerable object denoting a file name."
                     file)))
         file))
 
-  (define (prepend-btrfs-subvolume-file-name btrfs-subvolume-file-name file)
-    (if btrfs-subvolume-file-name
-        #~(string-append #$btrfs-subvolume-file-name #$file)
+  (define (prepend-store-directory-prefix store-directory-prefix file)
+    (if store-directory-prefix
+        #~(string-append #$store-directory-prefix #$file)
         file))
 
-  (prepend-btrfs-subvolume-file-name btrfs-subvolume-file-name
+  (prepend-store-directory-prefix store-directory-prefix
                                      (strip-mount-point mount-point file)))
 
 
@@ -135,14 +135,14 @@  file with the resolution provided in CONFIG."
            (_ #f)))))
 
 (define* (eye-candy config store-device store-mount-point
-                    #:key btrfs-store-subvolume-file-name system port)
+                    #:key store-directory-prefix system port)
   "Return a gexp that writes to PORT (a port-valued gexp) the 'grub.cfg' part
 concerned with graphics mode, background images, colors, and all that.
 STORE-DEVICE designates the device holding the store, and STORE-MOUNT-POINT is
 its mount point; these are used to determine where the background image and
 fonts must be searched for.  SYSTEM must be the target system string---e.g.,
-\"x86_64-linux\".  BTRFS-STORE-SUBVOLUME-FILE-NAME is the file name of the
-Btrfs subvolume, to be prepended to any store path, if any."
+\"x86_64-linux\".  STORE-DIRECTORY-PREFIX is a directory prefix to prepend to
+any store path."
   (define setup-gfxterm-body
     (let ((gfxmode
            (or (and-let* ((theme (bootloader-configuration-theme config))
@@ -181,12 +181,12 @@  fi~%" #+font-file)
   (define font-file
     (normalize-file (file-append grub "/share/grub/unicode.pf2")
                     store-mount-point
-                    btrfs-store-subvolume-file-name))
+                    store-directory-prefix))
 
   (define image
     (normalize-file (grub-background-image config)
                     store-mount-point
-                    btrfs-store-subvolume-file-name))
+                    store-directory-prefix))
 
   (and image
        #~(format #$port "
@@ -320,13 +320,13 @@  code."
                                   #:key
                                   (system (%current-system))
                                   (old-entries '())
-                                  btrfs-subvolume-file-name)
+                                  store-directory-prefix)
   "Return the GRUB configuration file corresponding to CONFIG, a
 <bootloader-configuration> object, and where the store is available at
-STORE-FS, a <file-system> object.  OLD-ENTRIES is taken to be a list
-of menu entries corresponding to old generations of the system.
-BTRFS-SUBVOLUME-FILE-NAME may be used to specify on which subvolume a
-Btrfs root file system resides."
+STORE-FS, a <file-system> object.  OLD-ENTRIES is taken to be a list of menu
+entries corresponding to old generations of the system.
+STORE-DIRECTORY-PREFIX may be used to specify a store prefix, as is required
+when booting a root file system on a Btrfs subvolume."
   (define all-entries
     (append entries (bootloader-configuration-menu-entries config)))
   (define (menu-entry->gexp entry)
@@ -336,17 +336,17 @@  Btrfs root file system resides."
            (arguments (menu-entry-linux-arguments entry))
            (kernel (normalize-file (menu-entry-linux entry)
                                    device-mount-point
-                                   btrfs-subvolume-file-name))
+                                   store-directory-prefix))
            (initrd (normalize-file (menu-entry-initrd entry)
                                    device-mount-point
-                                   btrfs-subvolume-file-name)))
+                                   store-directory-prefix)))
       ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
       ;; Use the right file names for KERNEL and INITRD in case
       ;; DEVICE-MOUNT-POINT is not "/", meaning that the store is on a
       ;; separate partition.
 
-      ;; When BTRFS-SUBVOLUME-FILE-NAME is defined, prepend it the kernel and
-      ;; initrd paths, to allow booting from a Btrfs subvolume.
+      ;; When STORE-DIRECTORY-PREFIX is defined, prepend it to the kernel and
+      ;; initrd paths, for example to allow booting from a Btrfs subvolume.
       #~(format port "menuentry ~s {
   ~a
   linux ~a ~a
@@ -360,7 +360,7 @@  Btrfs root file system resides."
     (eye-candy config
                (menu-entry-device (first all-entries))
                (menu-entry-device-mount-point (first all-entries))
-               #:btrfs-store-subvolume-file-name btrfs-subvolume-file-name
+               #:store-directory-prefix store-directory-prefix
                #:system system
                #:port #~port))
 
@@ -371,8 +371,8 @@  Btrfs root file system resides."
            (keymap* (and layout
                          (keyboard-layout-file layout #:grub grub)))
            (keymap (and keymap*
-                        (if btrfs-subvolume-file-name
-                            #~(string-append #$btrfs-subvolume-file-name
+                        (if store-directory-prefix
+                            #~(string-append #$store-directory-prefix
                                              #$keymap*)
                             keymap*))))
       #~(when #$keymap
diff --git a/gnu/system.scm b/gnu/system.scm
index d929187695..ac8bbd1d16 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -1118,7 +1118,7 @@  a list of <menu-entry>, to populate the \"old entries\" menu."
 
     (generate-config-file bootloader-conf (list entry)
                           #:old-entries old-entries
-                          #:btrfs-subvolume-file-name
+                          #:store-directory-prefix
 			  (btrfs-store-subvolume-file-name file-systems))))
 
 (define* (operating-system-boot-parameters os root-device
-- 
2.26.2