all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: Pierre Neidhardt <mail@ambrevar.xyz>, 37305@debbugs.gnu.org
Subject: [bug#37305] [PATCH v3] Allow booting from a Btrfs subvolume
Date: Wed, 20 May 2020 08:44:02 -0400	[thread overview]
Message-ID: <87wo56pyi5.fsf@gmail.com> (raw)
In-Reply-To: <87y2pohpqx.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Mon, 18 May 2020 23:55:50 +0200")

Hi Ludovic!

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

> Hi Maxim,
>
> Sorry for dropping the ball for sooo long.

No worries :-)

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

[...]

>> +++ b/gnu/build/linux-boot.scm
>> @@ -498,25 +498,13 @@ upon error."
>>    (define (root-mount-point? fs)
>>      (string=? (file-system-mount-point fs) "/"))
>>  
>> -  (define root-fs-type
>> -    (or (any (lambda (fs)
>> -               (and (root-mount-point? fs)
>> -                    (file-system-type fs)))
>> -             mounts)
>> -        "ext4"))
>> -
>> -  (define root-fs-flags
>> -    (mount-flags->bit-mask (or (any (lambda (fs)
>> -                                      (and (root-mount-point? fs)
>> -                                           (file-system-flags fs)))
>> -                                    mounts)
>> -                               '())))
>> -
>> -  (define root-fs-options
>> -    (any (lambda (fs)
>> -           (and (root-mount-point? fs)
>> -                (file-system-options fs)))
>> -         mounts))
>
> [...]
>
>> +             (root-fs (find root-mount-point? mounts))
>> +             (root-fs-type (or (and=> root-fs file-system-type)
>> +                               "ext4"))
>> +             (root-fs-device (and=> root-fs file-system-device))
>> +             (root-fs-flags (mount-flags->bit-mask
>> +                             (or (and=> root-fs file-system-flags)
>> +                                 '())))
>> +             (root-options (if root-fs
>> +                               (file-system-options root-fs)
>> +                               #f))
>
> I would tend to keep these as defines to make the ‘let*’ less
> intimidating, but it’s a detail.

It would only *appear* less intimidating ;-).  I personally prefer the
let* versions as the logic is more succinctly expressed (there is no
need for 'any' + lambdas, for example).

>> +        ;; XXX: Importing (guix utils) and using &fix-hint causes the
>> +        ;; following error when booting the init RAM disk: "ERROR: In
>> +        ;; procedure dynamic-func:\nIn procedure dynamic-pointer: Symbol not
>> +        ;; found: strverscmp", so we just embed the hint in the message.
>
> I think it should just be “FIXME: Use &fix-hint once it no longer pulls
> in (guix utils).”

Done!

>>>From 082934db68964890ebd2a2118fb44d66911844d3 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Sun, 14 Jul 2019 20:50:23 +0900
>> Subject: [PATCH 4/4] bootloader: grub: Allow booting from a Btrfs subvolume.
>>
>> * gnu/bootloader/grub.scm (strip-mount-point): Remove procedure.
>> (normalize-file): Add procedure.
>> (grub-configuration-file): New BTRFS-SUBVOLUME-FILE-NAME parameter.  When
>> defined, prepend its value to the kernel and initrd file names, using the
>> NORMALIZE-FILE procedure.  Adjust the call to EYE-CANDY to pass the
>> BTRFS-SUBVOLUME-FILE-NAME argument.  Normalize the KEYMAP file as well.
>> (eye-candy): Add a BTRFS-SUBVOLUME-FILE-NAME parameter, and use it, along with
>> the NORMALIZE-FILE procedure, to normalize the FONT-FILE and IMAGE nested
>> variables.  Adjust doc.
>> * gnu/bootloader/depthcharge.scm (depthcharge-configuration-file): Adapt.
>> * gnu/bootloader/extlinux.scm (extlinux-configuration-file): Likewise.
>> * gnu/system/file-systems.scm (btrfs-subvolume?)
>> (btrfs-store-subvolume-file-name): New procedures.
>> * gnu/system.scm (operating-system-bootcfg): Specify the Btrfs
>> subvolume file name the store resides on to the
>> `operating-system-bootcfg' procedure, using the new
>> BTRFS-SUBVOLUME-FILE-NAME argument.
>> * doc/guix.texi (File Systems): Add a Btrfs subsection to document the use of
>> subvolumes.
>> * gnu/tests/install.scm: Add test "btrfs-root-on-subvolume-os".
>
> Please list the entities added to ‘install.scm’.

Done!

>>  (define* (eye-candy config store-device store-mount-point
>> +                    btrfs-store-subvolume-file-name
>>                      #:key system port)
>
> I think ‘btrfs-store-subvolume-file-name’ should be a keyword argument.

Done!

>>  (define* (grub-configuration-file config entries
>>                                    #:key
>>                                    (system (%current-system))
>> -                                  (old-entries '()))
>> +                                  (old-entries '())
>> +                                  btrfs-subvolume-file-name)
>
> I wonder if we should just call it ‘store-directory-prefix’ or similar
> since, after all, it’s just about prepending a prefix, which could
> perhaps be useful for file systems other than Btrfs.
>
> Thoughts?

Perhaps, but given it's not yet clear whether another file system will
require similar support from GRUB, I'd rather keep things as explicit as
possible for now.

> Anyway, that’s great work, so I’ll be happy to finally see it committed
> in the coming days!

Thanks for the great words and for having a last look :-).

I've added a news entry and pushed to master as:

--8<---------------cut here---------------start------------->8---
489699c456 allow-booting-from-btrfs-subvolume news: Add entry for Btrfs subvolume boot support.
b460ba7992 bootloader: grub: Allow booting from a Btrfs subvolume.
fa35fb58c8 file-systems: Add helpers for parsing the options string into an alist.
281d80d8e5 linux-boot: Refactor boot-system.
--8<---------------cut here---------------end--------------->8---

Closing!  Thanks to Pierre and Ludovic for testing and reviewing.

Maxim




  reply	other threads:[~2020-05-20 12:45 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05  0:20 [bug#37305] [PATCH] Allow booting from a Btrfs subvolume Maxim Cournoyer
2019-09-08 16:10 ` Christopher Baines
2019-09-22 21:43 ` Ludovic Courtès
2020-02-12  8:47   ` Maxim Cournoyer
2020-02-13 20:27     ` [bug#37305] [PATCH V2] " Maxim Cournoyer
2020-02-14 17:22       ` Ludovic Courtès
2020-02-16  5:36         ` Maxim Cournoyer
2020-02-16 11:11           ` [bug#37305] Making system installation tests faster Ludovic Courtès
2020-02-18 13:37             ` Maxim Cournoyer
2020-02-18 21:27               ` Maxim Cournoyer
2020-03-07  4:01                 ` Maxim Cournoyer
2020-02-24 16:02           ` [bug#37305] [PATCH V2] Allow booting from a Btrfs subvolume Ludovic Courtès
2020-03-03  5:00             ` Maxim Cournoyer
2020-02-24 14:23         ` [bug#37305] [PATCH V3] " Maxim Cournoyer
2020-02-19  2:52 ` [bug#37305] Allow booting from a Btrfs subvolume [review part 2] Maxim Cournoyer
2020-02-20  9:55   ` Ludovic Courtès
2020-03-18 15:27     ` maxim.cournoyer
2020-05-17 13:29       ` Pierre Neidhardt
2020-05-17 16:13         ` [bug#37305] [PATCH v3] Allow booting from a Btrfs subvolume Maxim Cournoyer
2020-05-17 16:37           ` Pierre Neidhardt
2020-05-17 19:05             ` Pierre Neidhardt
2020-05-17 19:09               ` Pierre Neidhardt
2020-05-17 19:48                 ` Pierre Neidhardt
2020-05-18  1:16                   ` Maxim Cournoyer
2020-05-18  8:54                     ` Pierre Neidhardt
2020-05-17 20:22                 ` Pierre Neidhardt
2020-05-18  0:49                   ` Maxim Cournoyer
2020-05-18 21:55           ` Ludovic Courtès
2020-05-20 12:44             ` Maxim Cournoyer [this message]
2020-05-20 12:44             ` bug#37305: " Maxim Cournoyer
2020-05-20 13:29               ` [bug#37305] " Pierre Neidhardt
2020-05-20 22:03               ` Ludovic Courtès
2020-05-21  6:58                 ` Pierre Neidhardt
2020-05-28  4:30                   ` Maxim Cournoyer
2020-05-28  8:26                     ` Pierre Neidhardt
2020-05-29 21:14                       ` Maxim Cournoyer
2020-05-28 12:30                     ` Ludovic Courtès
2020-05-30  2:00                       ` Maxim Cournoyer
2020-05-30  7:32                         ` Pierre Neidhardt
2020-05-30  7:32                         ` Pierre Neidhardt
2020-05-31  2:44                           ` Maxim Cournoyer
2020-05-31  7:32                             ` Pierre Neidhardt
2020-05-17 14:03       ` [bug#37305] Allow booting from a Btrfs subvolume [review part 2] Pierre Neidhardt
2020-05-17 16:16         ` Maxim Cournoyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wo56pyi5.fsf@gmail.com \
    --to=maxim.cournoyer@gmail.com \
    --cc=37305@debbugs.gnu.org \
    --cc=ludo@gnu.org \
    --cc=mail@ambrevar.xyz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.