GNU bug report logs - #57496
[PATCH 0/2] Add support for chain-loader

Previous Next

Package: guix-patches;

Reported by: typ22 <at> foxmail.com

Date: Wed, 31 Aug 2022 05:26:02 UTC

Severity: normal

Tags: patch

Done: Julien Lepiller <julien <at> lepiller.eu>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 05:26:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to typ22 <at> foxmail.com:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Wed, 31 Aug 2022 05:26:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: guix-patches <at> gnu.org
Cc: tiantian <typ22 <at> foxmail.com>
Subject: [PATCH 0/2] Add support for chain-loader
Date: Wed, 31 Aug 2022 13:23:06 +0800
From: tiantian <typ22 <at> foxmail.com>

Hi Guix,

I just started using guix system soon. I used  archlinux before.
Now I want to move from archlinux to guix system. It took me
about 2 years to move from windows to archlinux, and now it may
take me a long time to complete the moving from ArchLinx to guix system.
During this period, guix system and archlinux will coexist on my computer.
Therefore, it is necessary for me to easily enter another system in
the boot interface.
So I want to add support for chain-loader in menu-entry of guix system.

Why do I want to use chainloader instead of linux and initrd?

1. guix system and archlinux are independent of each other in my computer.
They are just on the same computer, but they have their own disk partitions.
I also don't want them to interfere with each other.
(I have no money to buy more computers. I just have one computer.)

2. If using linux and initrd, modify the boot arguments of one of them,
which must be in the other system. I think it is strange and inconvenient.
It's also not in line with the above idea of mutual independence and
non-interference.

3. I am a novice. If an unexpected event occurs when using linux and initrd,
I can't repair it quickly and timely.

Is the chain-loader free?

The chain-loader is a function of grub, and grub's protocol is GPL3.
I think it is free. Although chain-loader can boot nonfree system like windows,
it's like nonfree software can run on linux-libre, and linux-libre is free.

If Guix think it's nonfree, I will only keep these changes in my computer.

Here I want to express my thanks to Josselin Poiret. When I tried to start
this task, I found that there was a bug with the menu-entry. When the device
of menu-entry is UUID or file-system-label, an error will be reported by
'guix system' command. I can't fix it. Then I reported this bug in bug#57307.
Thanks to Josselin Poiret fixing the bug, I can continue this task.

My English is not good, I mainly rely on translation software. If I have any
grammar problems and tone problems, please forgive me. I am sending these emails
with respect.

Thanks,
tiantian


tiantian (2):
  gnu: bootloader: Extend `<menu-entry>' for chain-loader.
  gnu: bootloader: grub: Add support for chain-loader.

 doc/guix.texi           | 15 +++++++++
 gnu/bootloader.scm      | 40 ++++++++++++++++++----
 gnu/bootloader/grub.scm | 73 ++++++++++++++++++++++++-----------------
 3 files changed, 92 insertions(+), 36 deletions(-)


base-commit: 6beadc82df204f315d06ea35f2e232bb32f8e440
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 05:30:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: tiantian <typ22 <at> foxmail.com>
Subject: [PATCH 2/2] gnu: bootloader: grub: Add support for chain-loader.
Date: Wed, 31 Aug 2022 13:27:49 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader/grub.scm (grub-configuration-file): Add support for
chain-loader.
---
 gnu/bootloader/grub.scm | 73 ++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 4f18c9b518..7283257354 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -374,44 +374,57 @@ (define* (grub-configuration-file config entries
     (let ((label (menu-entry-label entry))
           (linux (menu-entry-linux entry))
           (device (menu-entry-device entry))
-          (device-mount-point (menu-entry-device-mount-point entry)))
-      (if linux
-          (let ((arguments (menu-entry-linux-arguments entry))
-                (linux (normalize-file linux
-                                       device-mount-point
-                                       store-directory-prefix))
-                (initrd (normalize-file (menu-entry-initrd entry)
-                                        device-mount-point
-                                        store-directory-prefix)))
-         ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
-         ;; Use the right file names for LINUX 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 linux and
-         ;; initrd paths, to allow booting from a Btrfs subvolume.
-         #~(format port "menuentry ~s {
+          (device-mount-point (menu-entry-device-mount-point entry))
+          (multiboot-kernel (menu-entry-multiboot-kernel entry))
+          (chain-loader (menu-entry-chain-loader entry)))
+      (cond
+       (linux
+        (let ((arguments (menu-entry-linux-arguments entry))
+              (linux (normalize-file linux
+                                     device-mount-point
+                                     store-directory-prefix))
+              (initrd (normalize-file (menu-entry-initrd entry)
+                                      device-mount-point
+                                      store-directory-prefix)))
+          ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
+          ;; Use the right file names for LINUX 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 linux and
+          ;; initrd paths, to allow booting from a Btrfs subvolume.
+          #~(format port "menuentry ~s {
   ~a
   linux ~a ~a
   initrd ~a
 }~%"
-                   #$label
-                   #$(grub-root-search device linux)
-                   #$linux (string-join (list #$@arguments))
-                   #$initrd))
-          (let ((kernel (menu-entry-multiboot-kernel entry))
-                (arguments (menu-entry-multiboot-arguments entry))
-                (modules (menu-entry-multiboot-modules entry))
-                (root-index 1))            ; XXX EFI will need root-index 2
-        #~(format port "
+                    #$label
+                    #$(grub-root-search device linux)
+                    #$linux (string-join (list #$@arguments))
+                    #$initrd)))
+       (multiboot-kernel
+        (let ((kernel (menu-entry-multiboot-kernel entry))
+              (arguments (menu-entry-multiboot-arguments entry))
+              (modules (menu-entry-multiboot-modules entry))
+              (root-index 1))            ; XXX EFI will need root-index 2
+          #~(format port "
 menuentry ~s {
   multiboot ~a root=device:hd0s~a~a~a
+}~%"
+                    #$label
+                    #$kernel
+                    #$root-index (string-join (list #$@arguments) " " 'prefix)
+                    (string-join (map string-join '#$modules)
+                                 "\n  module " 'prefix))))
+       (chain-loader
+        #~(format port "
+menuentry ~s {
+  ~a
+  chainloader ~a
 }~%"
                   #$label
-                  #$kernel
-                  #$root-index (string-join (list #$@arguments) " " 'prefix)
-                  (string-join (map string-join '#$modules)
-                               "\n  module " 'prefix))))))
+                  #$(grub-root-search device chain-loader)
+                  #$chain-loader)))))
 
   (define (crypto-devices)
     (define (crypto-device->cryptomount dev)
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 05:30:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: tiantian <typ22 <at> foxmail.com>
Subject: [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.
Date: Wed, 31 Aug 2022 13:27:48 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
* doc/guix.texi (Bootloader Configuration): Document it.
---
 doc/guix.texi      | 15 +++++++++++++++
 gnu/bootloader.scm | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 957b9a668e..69dcd9e7b9 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -37536,6 +37536,21 @@ Bootloader Configuration
              @dots{}))
 @end lisp
 
+@item @code{chain-loader} (default: @code{#f})
+A string that any accepted by @code{chainloader}. If there are items
+other than @code{label} and @code{device}, it will have no effect.
+
+@lisp
+(bootloader
+  (bootloader-configuration
+  ;; @dots{}
+    (menu-entries
+      (list
+        (menu-entry
+          (label "GNU/Linux")
+          (chain-loader "(hd0,gpt1)/EFI/GNULinux/grubx64.efi"))))))
+@end lisp
+
 @end table
 @end deftp
 
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 77c05e8946..41f690a4dc 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
             menu-entry-multiboot-kernel
             menu-entry-multiboot-arguments
             menu-entry-multiboot-modules
+            menu-entry-chain-loader
 
             menu-entry->sexp
             sexp->menu-entry
@@ -104,8 +105,11 @@ (define-record-type* <menu-entry>
   (multiboot-arguments menu-entry-multiboot-arguments
                        (default '()))      ; list of string-valued gexps
   (multiboot-modules menu-entry-multiboot-modules
-                     (default '())))       ; list of multiboot commands, where
+                     (default '()))        ; list of multiboot commands, where
                                            ; a command is a list of <string>
+  (chain-loader     menu-entry-chain-loader
+                    (default #f)))         ; string, path of efi file
+
 
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
@@ -117,8 +121,15 @@ (define (menu-entry->sexp entry)
        `(label ,(file-system-label->string label)))
       (_ device)))
   (match entry
-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
-                     ())
+    ;; Fields of <menu-entry> in the patterns is incomplete,
+    ;; resulting in menu-entry with chain-loader incorrectly
+    ;; matching the first.
+    ;; To keep pattern is short and keep matching order,
+    ;; judge an important field in each pattern.
+    ;; Such as kernel.
+    (($ <menu-entry> label device mount-point
+                     (? identity linux) linux-arguments initrd
+                     #f ())
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
@@ -127,14 +138,22 @@ (define (menu-entry->sexp entry)
                   (linux-arguments ,linux-arguments)
                   (initrd ,initrd)))
     (($ <menu-entry> label device mount-point #f () #f
-                     multiboot-kernel multiboot-arguments multiboot-modules)
+                     (? identity multiboot-kernel) multiboot-arguments
+                     multiboot-modules)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
                   (multiboot-kernel ,multiboot-kernel)
                   (multiboot-arguments ,multiboot-arguments)
-                  (multiboot-modules ,multiboot-modules)))))
+                  (multiboot-modules ,multiboot-modules)))
+    (($ <menu-entry> label device mount-point #f () #f #f () ()
+                     (? identity chain-loader))
+     `(menu-entry (version 0)
+                  (label ,label)
+                  (device ,(device->sexp device))
+                  (device-mount-point ,mount-point)
+                  (chain-loader ,chain-loader)))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
@@ -171,7 +190,16 @@ (define (sexp->menu-entry sexp)
       (device-mount-point mount-point)
       (multiboot-kernel multiboot-kernel)
       (multiboot-arguments multiboot-arguments)
-      (multiboot-modules multiboot-modules)))))
+      (multiboot-modules multiboot-modules)))
+    (('menu-entry ('version 0)
+                  ('label label) ('device device)
+                  ('device-mount-point mount-point)
+                  ('chain-loader chain-loader) _ ...)
+     (menu-entry
+      (label label)
+      (device (sexp->device device))
+      (device-mount-point mount-point)
+      (chain-loader chain-loader)))))
 
 
 ;;;
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 07:20:02 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org, typ22 <at> foxmail.com, 57496 <at> debbugs.gnu.org
Cc: tiantian <typ22 <at> foxmail.com>
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.
Date: Wed, 31 Aug 2022 09:18:44 +0200
[Message part 1 (text/plain, inline)]
Hi tiantian!

Thanks for the patches. It's a bit hard to follow since I'm not at a computer, but here are some preliminary remarks.

First, thanks for documenting the change in the manual, it's not something even I think about all the time ^^'. We'll have to rework the English a bit but it's understandable :)

I'm wondering why there are two patches? The first patch below seems documented but the second patch does not have a changelog text. Shouldn't they be merged? They seem to be for the same thing.

From what I understand, specifying chainloader with at least linux or linux-multiboot will lead to chainloader being silently dropped. Maybe we should have an error message instead?

I'm not too familiar with chainloading. Is using grub partition names like (hd0,gpt1) the only way? We try to discourage using these names as they can easily vary between reboots and your system unbootable.

Otherwise, it looks fine at first glance, but untested :)

Le 31 août 2022 07:27:48 GMT+02:00, typ22 <at> foxmail.com a écrit :
>From: tiantian <typ22 <at> foxmail.com>
>
>* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
>(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
>* doc/guix.texi (Bootloader Configuration): Document it.
>---
> doc/guix.texi      | 15 +++++++++++++++
> gnu/bootloader.scm | 40 ++++++++++++++++++++++++++++++++++------
> 2 files changed, 49 insertions(+), 6 deletions(-)
>
>diff --git a/doc/guix.texi b/doc/guix.texi
>index 957b9a668e..69dcd9e7b9 100644
>--- a/doc/guix.texi
>+++ b/doc/guix.texi
>@@ -37536,6 +37536,21 @@ Bootloader Configuration
>              @dots{}))
> @end lisp
> 
>+@item @code{chain-loader} (default: @code{#f})
>+A string that any accepted by @code{chainloader}. If there are items
>+other than @code{label} and @code{device}, it will have no effect.
>+
>+@lisp
>+(bootloader
>+  (bootloader-configuration
>+  ;; @dots{}
>+    (menu-entries
>+      (list
>+        (menu-entry
>+          (label "GNU/Linux")
>+          (chain-loader "(hd0,gpt1)/EFI/GNULinux/grubx64.efi"))))))
>+@end lisp
>+
> @end table
> @end deftp
> 
>diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
>index 77c05e8946..41f690a4dc 100644
>--- a/gnu/bootloader.scm
>+++ b/gnu/bootloader.scm
>@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
>             menu-entry-multiboot-kernel
>             menu-entry-multiboot-arguments
>             menu-entry-multiboot-modules
>+            menu-entry-chain-loader
> 
>             menu-entry->sexp
>             sexp->menu-entry
>@@ -104,8 +105,11 @@ (define-record-type* <menu-entry>
>   (multiboot-arguments menu-entry-multiboot-arguments
>                        (default '()))      ; list of string-valued gexps
>   (multiboot-modules menu-entry-multiboot-modules
>-                     (default '())))       ; list of multiboot commands, where
>+                     (default '()))        ; list of multiboot commands, where
>                                            ; a command is a list of <string>
>+  (chain-loader     menu-entry-chain-loader
>+                    (default #f)))         ; string, path of efi file
>+
> 
> (define (menu-entry->sexp entry)
>   "Return ENTRY serialized as an sexp."
>@@ -117,8 +121,15 @@ (define (menu-entry->sexp entry)
>        `(label ,(file-system-label->string label)))
>       (_ device)))
>   (match entry
>-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
>-                     ())
>+    ;; Fields of <menu-entry> in the patterns is incomplete,
>+    ;; resulting in menu-entry with chain-loader incorrectly
>+    ;; matching the first.
>+    ;; To keep pattern is short and keep matching order,
>+    ;; judge an important field in each pattern.
>+    ;; Such as kernel.
>+    (($ <menu-entry> label device mount-point
>+                     (? identity linux) linux-arguments initrd
>+                     #f ())
>      `(menu-entry (version 0)
>                   (label ,label)
>                   (device ,(device->sexp device))
>@@ -127,14 +138,22 @@ (define (menu-entry->sexp entry)
>                   (linux-arguments ,linux-arguments)
>                   (initrd ,initrd)))
>     (($ <menu-entry> label device mount-point #f () #f
>-                     multiboot-kernel multiboot-arguments multiboot-modules)
>+                     (? identity multiboot-kernel) multiboot-arguments
>+                     multiboot-modules)
>      `(menu-entry (version 0)
>                   (label ,label)
>                   (device ,(device->sexp device))
>                   (device-mount-point ,mount-point)
>                   (multiboot-kernel ,multiboot-kernel)
>                   (multiboot-arguments ,multiboot-arguments)
>-                  (multiboot-modules ,multiboot-modules)))))
>+                  (multiboot-modules ,multiboot-modules)))
>+    (($ <menu-entry> label device mount-point #f () #f #f () ()
>+                     (? identity chain-loader))
>+     `(menu-entry (version 0)
>+                  (label ,label)
>+                  (device ,(device->sexp device))
>+                  (device-mount-point ,mount-point)
>+                  (chain-loader ,chain-loader)))))
> 
> (define (sexp->menu-entry sexp)
>   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
>@@ -171,7 +190,16 @@ (define (sexp->menu-entry sexp)
>       (device-mount-point mount-point)
>       (multiboot-kernel multiboot-kernel)
>       (multiboot-arguments multiboot-arguments)
>-      (multiboot-modules multiboot-modules)))))
>+      (multiboot-modules multiboot-modules)))
>+    (('menu-entry ('version 0)
>+                  ('label label) ('device device)
>+                  ('device-mount-point mount-point)
>+                  ('chain-loader chain-loader) _ ...)
>+     (menu-entry
>+      (label label)
>+      (device (sexp->device device))
>+      (device-mount-point mount-point)
>+      (chain-loader chain-loader)))))
> 
> >
> ;;;
>-- 
>2.37.2
>
>
>
>
[Message part 2 (text/html, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 07:21:02 GMT) Full text and rfc822 format available.

Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 07:47:02 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: 57496 <at> debbugs.gnu.org, typ22 <at> foxmail.com
Cc: tiantian <typ22 <at> foxmail.com>
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.
Date: Wed, 31 Aug 2022 09:45:52 +0200

Le 31 août 2022 09:18:44 GMT+02:00, Julien Lepiller <julien <at> lepiller.eu> a écrit :
>Hi tiantian!
>
>Thanks for the patches. It's a bit hard to follow since I'm not at a computer, but here are some preliminary remarks.
>
>First, thanks for documenting the change in the manual, it's not something even I think about all the time ^^'. We'll have to rework the English a bit but it's understandable :)
>
>I'm wondering why there are two patches? The first patch below seems documented but the second patch does not have a changelog text. Shouldn't they be merged? They seem to be for the same thing.

Nevesmind, I don't know how I missed it, there is a changelog for that patch. I still think it could be merged with the first. Or any reason to keep it separate?

>
>From what I understand, specifying chainloader with at least linux or linux-multiboot will lead to chainloader being silently dropped. Maybe we should have an error message instead?
>
>I'm not too familiar with chainloading. Is using grub partition names like (hd0,gpt1) the only way? We try to discourage using these names as they can easily vary between reboots and your system unbootable.
>
>Otherwise, it looks fine at first glance, but untested :)
>
>Le 31 août 2022 07:27:48 GMT+02:00, typ22 <at> foxmail.com a écrit :
>>From: tiantian <typ22 <at> foxmail.com>
>>
>>* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
>>(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
>>* doc/guix.texi (Bootloader Configuration): Document it.
>>---
>> doc/guix.texi      | 15 +++++++++++++++
>> gnu/bootloader.scm | 40 ++++++++++++++++++++++++++++++++++------
>> 2 files changed, 49 insertions(+), 6 deletions(-)
>>
>>diff --git a/doc/guix.texi b/doc/guix.texi
>>index 957b9a668e..69dcd9e7b9 100644
>>--- a/doc/guix.texi
>>+++ b/doc/guix.texi
>>@@ -37536,6 +37536,21 @@ Bootloader Configuration
>>              @dots{}))
>> @end lisp
>> 
>>+@item @code{chain-loader} (default: @code{#f})
>>+A string that any accepted by @code{chainloader}. If there are items
>>+other than @code{label} and @code{device}, it will have no effect.
>>+
>>+@lisp
>>+(bootloader
>>+  (bootloader-configuration
>>+  ;; @dots{}
>>+    (menu-entries
>>+      (list
>>+        (menu-entry
>>+          (label "GNU/Linux")
>>+          (chain-loader "(hd0,gpt1)/EFI/GNULinux/grubx64.efi"))))))
>>+@end lisp
>>+
>> @end table
>> @end deftp
>> 
>>diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
>>index 77c05e8946..41f690a4dc 100644
>>--- a/gnu/bootloader.scm
>>+++ b/gnu/bootloader.scm
>>@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
>>             menu-entry-multiboot-kernel
>>             menu-entry-multiboot-arguments
>>             menu-entry-multiboot-modules
>>+            menu-entry-chain-loader
>> 
>>             menu-entry->sexp
>>             sexp->menu-entry
>>@@ -104,8 +105,11 @@ (define-record-type* <menu-entry>
>>   (multiboot-arguments menu-entry-multiboot-arguments
>>                        (default '()))      ; list of string-valued gexps
>>   (multiboot-modules menu-entry-multiboot-modules
>>-                     (default '())))       ; list of multiboot commands, where
>>+                     (default '()))        ; list of multiboot commands, where
>>                                            ; a command is a list of <string>
>>+  (chain-loader     menu-entry-chain-loader
>>+                    (default #f)))         ; string, path of efi file
>>+
>> 
>> (define (menu-entry->sexp entry)
>>   "Return ENTRY serialized as an sexp."
>>@@ -117,8 +121,15 @@ (define (menu-entry->sexp entry)
>>        `(label ,(file-system-label->string label)))
>>       (_ device)))
>>   (match entry
>>-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
>>-                     ())
>>+    ;; Fields of <menu-entry> in the patterns is incomplete,
>>+    ;; resulting in menu-entry with chain-loader incorrectly
>>+    ;; matching the first.
>>+    ;; To keep pattern is short and keep matching order,
>>+    ;; judge an important field in each pattern.
>>+    ;; Such as kernel.
>>+    (($ <menu-entry> label device mount-point
>>+                     (? identity linux) linux-arguments initrd
>>+                     #f ())
>>      `(menu-entry (version 0)
>>                   (label ,label)
>>                   (device ,(device->sexp device))
>>@@ -127,14 +138,22 @@ (define (menu-entry->sexp entry)
>>                   (linux-arguments ,linux-arguments)
>>                   (initrd ,initrd)))
>>     (($ <menu-entry> label device mount-point #f () #f
>>-                     multiboot-kernel multiboot-arguments multiboot-modules)
>>+                     (? identity multiboot-kernel) multiboot-arguments
>>+                     multiboot-modules)
>>      `(menu-entry (version 0)
>>                   (label ,label)
>>                   (device ,(device->sexp device))
>>                   (device-mount-point ,mount-point)
>>                   (multiboot-kernel ,multiboot-kernel)
>>                   (multiboot-arguments ,multiboot-arguments)
>>-                  (multiboot-modules ,multiboot-modules)))))
>>+                  (multiboot-modules ,multiboot-modules)))
>>+    (($ <menu-entry> label device mount-point #f () #f #f () ()
>>+                     (? identity chain-loader))
>>+     `(menu-entry (version 0)
>>+                  (label ,label)
>>+                  (device ,(device->sexp device))
>>+                  (device-mount-point ,mount-point)
>>+                  (chain-loader ,chain-loader)))))
>> 
>> (define (sexp->menu-entry sexp)
>>   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
>>@@ -171,7 +190,16 @@ (define (sexp->menu-entry sexp)
>>       (device-mount-point mount-point)
>>       (multiboot-kernel multiboot-kernel)
>>       (multiboot-arguments multiboot-arguments)
>>-      (multiboot-modules multiboot-modules)))))
>>+      (multiboot-modules multiboot-modules)))
>>+    (('menu-entry ('version 0)
>>+                  ('label label) ('device device)
>>+                  ('device-mount-point mount-point)
>>+                  ('chain-loader chain-loader) _ ...)
>>+     (menu-entry
>>+      (label label)
>>+      (device (sexp->device device))
>>+      (device-mount-point mount-point)
>>+      (chain-loader chain-loader)))))
>> 
>> >>
>> ;;;
>>-- 
>>2.37.2
>>
>>
>>
>>




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 19:35:02 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: tiantian <typ22 <at> foxmail.com>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>'
 for chain-loader.
Date: Wed, 31 Aug 2022 21:34:06 +0200
Le Thu, 1 Sep 2022 01:55:34 +0800,
tiantian <typ22 <at> foxmail.com> a écrit :

> Dear Mr/Ms Lepiller,
> 
> I'm sorry. I didn't notice the wrong sender name.

You don't have to apologize. I received your email and I didn't even
notice the sender name :)

> 
> Because my patch was replied so quickly for the first time, I am
> excited and hope to reply as soon as possible.
> 
> After the last installation of icedove, the mail client on my
> computer, I don't know why icedove lost all configuration information
> this time, so I log in again. Icedove defaults to the desktop user
> name I set for testing the guix system. In a hurry, I didn't notice
> the mistake.
> 
> I checked the contents of the email and tried to make the meaning
> accurate. Without noticing that the name of the sender of the mail is
> wrong.
> 
> I'm not sure if the previous mail was intercepted in the trash, so I
> forward it.
> 
> My mistake was really stupid. I sincerely apologize for disturbing
> you.
> 
> Sincerely,
> tiantian
> 
> -------- Forwarded Message --------
> Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend
> `<menu-entry>' for chain-loader. Date: Wed, 31 Aug 2022 20:22:55 +0800
> From: user in guix system <typ22 <at> foxmail.com>
> To: Julien Lepiller <julien <at> lepiller.eu>, 57496 <at> debbugs.gnu.org
> 
> Hi,
> 
> On 2022/8/31 15:18, Julien Lepiller wrote:
> > Hi tiantian!
> >
> > Thanks for the patches. It's a bit hard to follow since I'm not at a
> > computer, but here are some preliminary remarks.
> >
> > First, thanks for documenting the change in the manual, it's not
> > something even I think about all the time ^^'. We'll have to rework
> > the English a bit but it's understandable :)
> >  
> 
> My English is not good. Thank you for correcting the document.

Let's try something like this:

@item @code{chain-loader} (default: @code{#f})
A string that can be accepted by @code{grub}'s @code{chainloader}
directive. This has no effect if either @code{linux} or
@code{linux-multiboot} fields are specified. The following is an
example of chainloading a different GNU/Linux system.

> 
> > I'm wondering why there are two patches? The first patch below seems
> > documented but the second patch does not have a changelog text.
> > Shouldn't they be merged? They seem to be for the same thing.
> >  
> 
> I have little submission experience and my English isn't good, so I
> refer to the submission history of multiboot. As you can see, my
> submission also imitates it. However, I did not modify
> <boot-parameters> like it. On the one hand, I did not understand the
> role of <boot-parameters>. on the other hand, it has successfully
> passed the tests on my computer, such as reconfigure,
> switch-generation and roll-back.

I didn't know about boot-parameters. It sounds like they are related to
guix deploy, which I don't use. I think we can still go with your
patch, and later add chainloading in boot-parameters if needed.

> 
> If it is better to merge into the first, there is no problem. How can
> I do this? Should I send v2 here after local merge, or other?
> 
> commit 1244491a0d5334e1589159a2ff67bbc967b9648b
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date:   Tue May 26 18:09:01 2020 +0200
> 
>      bootloader: grub: Add support for multiboot.
> 
>      * gnu/bootloader/grub.scm (grub-configuration-file): Add support
> for multiboot.
> 
> commit 912b857ede450828805e09bb718658f79c40703a
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date:   Tue May 26 17:38:30 2020 +0200
> 
>      system: Add 'multiboot-modules' field to <boot-parameters>.
> 
>      * gnu/system.scm (<boot-parameters>)[multiboot-modules]: New
> field. (read-boot-parameters): Initialize it.
>      (operating-system-multiboot-modules, hurd-multiboot-modules):
> New procedure. (operating-system-boot-parameters): Cater for
> multiboot the Hurd and initialize it; avoid initrd in that case.
>      (operating-system-kernel-file): Cater for for Gnumach (the Hurd)
> besides Linux. (boot-parameters->menu-entry): Use it to support a
> multiboot <menu-entry>.
> 
> commit 21acd8d6c11b85d06c82b168807b35cb7d2d0adf
> Author: Jan (janneke) Nieuwenhuizen <janneke <at> gnu.org>
> Date:   Tue May 26 16:54:18 2020 +0200
> 
>      bootloader: Extend `<menu-entry>' for multiboot.
> 
>      * gnu/bootloader.scm
> (<menu-entry>)[multiboot-kernel,multiboot-arguments,
> multiboot-modules]: New fields. [linux,initrd]: Add default value
> '#f'. (menu-entry->sexp, sexp->menu-entry): Support multiboot entry.
>      * doc/guix.texi (Bootloader Configuration): Document them.
> 

OK, I see now. I don't really understand why they were separate, but
let's keep them separate for now.

> >  From what I understand, specifying chainloader with at least linux
> > or linux-multiboot will lead to chainloader being silently dropped.
> > Maybe we should have an error message instead?
> >  
> 
> Yes. Thank you for asking this question. I didn't think about it
> before. What I can think of now is to complete pattern as follows.
> When the chainloader and Linux or multiboot are specified at the same
> time, it will report an error message.
> 
> But I feel that this is not elegant, it will make code difficult to
> read. I am a beginner of scheme. Could you give me some advice?
> 
> --- >8 ---  
> 
>   (match entry
>      (($ <menu-entry> label device mount-point linux linux-arguments
> initrd #f () () #f)
>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
>                    (device-mount-point ,mount-point)
>                    (linux ,linux)
>                    (linux-arguments ,linux-arguments)
>                    (initrd ,initrd)))
>      (($ <menu-entry> label device mount-point #f () #f
>                       multiboot-kernel multiboot-arguments
> multiboot-modules #f)
>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
>                    (device-mount-point ,mount-point)
>                    (multiboot-kernel ,multiboot-kernel)
>                    (multiboot-arguments ,multiboot-arguments)
>                    (multiboot-modules ,multiboot-modules)))
>      (($ <menu-entry> label device mount-point #f () #f #f () ()
>                       chain-loader)
>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
>                    (device-mount-point ,mount-point)
>                    (chain-loader ,chain-loader))))
> 
> --- <8 ---

I prefer this variant where the pattern is explicit.

As with what we have today, if the user specifies more than one of
linux, linux-multiboot and chainloader, they get an unhelpful "no
matching pattern" error.

This could be done later if you don't have time, but I would suggest to
fix it by adding a default case that matches all incorrect cases, like
so:

(_ (raise (condition (&message (message (G_ "Your error message
here"))))))

Have a look at other "&message" conditions for inspiration.

Also I noticed that if all of linux, linux-multiboot and chainloader
are #f, then the first pattern matches and will lead to a different
error message. I haven't tested so I'm not sure what we get, but it
might be interresting to match on all of them being #f, and print a
different message. Again, this can be done later.

> 
> > I'm not too familiar with chainloading. Is using grub partition
> > names like (hd0,gpt1) the only way? We try to discourage using
> > these names as they can easily vary between reboots and your system
> > unbootable. 
> 
> It can also use device to specify the disk partition. The following is
> the menu-entry that I am using.
> 
> --- >8 ---  
> 
> (menu-entries
> (list
>   (menu-entry
>    (label "ArchLinux")
>    (device (uuid "1C31-A17C" 'fat))
>    (chain-loader "/EFI/ArchLinux/grubx64.efi"))))
> 
> --- <8 ---
> 
> The examples in the document were written before the bug#57307 was
> fixed.  At that time, only this example passed the test on my
> computer. I didn't take into account that the example was bad. I'm
> sorry.

This new example is perfect. Could you add it to your next patch?

> 
> I'm sorry, because I don't know how to paste the code snippet in the
> mail. After seeing your reply, after a long search, no good example
> was found. I don't know if there is a problem when I write the code
> snippet like this. If there is any problem, I'm sorry.

They came out fine. I don't use anything fancy to read emails, so I
don't really care. I think emacs might have something to show snippets
of code better.

> 
> Thank you very much for checking and correcting my patches.

Could you send a v2 with the changes we discussed so far?

Thanks,
Julien

> 
> Thanks,
> tiantian
> 
> 





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Wed, 31 Aug 2022 19:53:02 GMT) Full text and rfc822 format available.

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: typ22 <at> foxmail.com, 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 0/2] Add support for chain-loader
Date: Wed, 31 Aug 2022 21:52:04 +0200
[Message part 1 (text/plain, inline)]
> Why do I want to use chainloader instead of linux and initrd?
>
> 1. guix system and archlinux are independent of each other in my computer.
> They are just on the same computer, but they have their own disk partitions.
> I also don't want them to interfere with each other.
> (I have no money to buy more computers. I just have one computer.)
Sounds a practical system to me.

On 31-08-2022 07:23, typ22 <at> foxmail.com wrote:
> Is the chain-loader free?
>
> The chain-loader is a function of grub, and grub's protocol is GPL3.
> I think it is free. Although chain-loader can boot nonfree system like windows,
> it's like nonfree software can run on linux-libre, and linux-libre is free.
>
> If Guix think it's nonfree, I will only keep these changes in my computer.

I do believe it counts as free, especially given that it can be used to 
boot into other free systems (e.g., Trisquel) and that you example usage 
is for booting into a largely-free system.

Greetings,
Maxime.
[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]
[OpenPGP_signature (application/pgp-signature, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Thu, 01 Sep 2022 03:51:01 GMT) Full text and rfc822 format available.

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

From: tiantian <typ22 <at> foxmail.com>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>'
 for chain-loader.
Date: Thu, 01 Sep 2022 10:33:51 +0800
Hi,

Julien Lepiller <julien <at> lepiller.eu> writes:

> Le Thu, 1 Sep 2022 01:55:34 +0800,
> tiantian <typ22 <at> foxmail.com> a écrit :
>
>> Dear Mr/Ms Lepiller,
>> 
>> I'm sorry. I didn't notice the wrong sender name.
>
> You don't have to apologize. I received your email and I didn't even
> notice the sender name :)
>

Thank you for your generosity.

>
> Let's try something like this:
>
> @item @code{chain-loader} (default: @code{#f})
> A string that can be accepted by @code{grub}'s @code{chainloader}
> directive. This has no effect if either @code{linux} or
> @code{linux-multiboot} fields are specified. The following is an
> example of chainloading a different GNU/Linux system.
>

Thank you for your help. I will change it in next patch.

But I have a little doubt. 'linux-multiboot' has never appeared in the
documentation. Will it be difficult to understand the document? I don't
know much about multiboot. I haven't seen the "linux-" prefix in
multiboot before. Does multiboot only support linux?

>
> OK, I see now. I don't really understand why they were separate, but
> let's keep them separate for now.
>

OK, I will keep them separate.

>
> I prefer this variant where the pattern is explicit.
>
> As with what we have today, if the user specifies more than one of
> linux, linux-multiboot and chainloader, they get an unhelpful "no
> matching pattern" error.
>
> This could be done later if you don't have time, but I would suggest to
> fix it by adding a default case that matches all incorrect cases, like
> so:
>
> (_ (raise (condition (&message (message (G_ "Your error message
> here"))))))
>
> Have a look at other "&message" conditions for inspiration.
>
> Also I noticed that if all of linux, linux-multiboot and chainloader
> are #f, then the first pattern matches and will lead to a different
> error message. I haven't tested so I'm not sure what we get, but it
> might be interresting to match on all of them being #f, and print a
> different message. Again, this can be done later.
>

Thank you for your suggestions. I will use in the pattern to specify all
fields of <menu-entry> in next patch.
I didn't know how to throw an error message before. I may need to spend
time reading code and learning. If possible, I will implement it in v3
patch.

>> It can also use device to specify the disk partition. The following is
>> the menu-entry that I am using.
>> 
>> --- >8 ---  
>> 
>> (menu-entries
>> (list
>>   (menu-entry
>>    (label "ArchLinux")
>>    (device (uuid "1C31-A17C" 'fat))
>>    (chain-loader "/EFI/ArchLinux/grubx64.efi"))))
>> 
>> --- <8 ---
>> 
>> The examples in the document were written before the bug#57307 was
>> fixed.  At that time, only this example passed the test on my
>> computer. I didn't take into account that the example was bad. I'm
>> sorry.
>
> This new example is perfect. Could you add it to your next patch?
>

No problem.
In order to avoid the possible controversy over the Linux distribution,
I will change ArchLinux to GNU/Linux.

>
> Could you send a v2 with the changes we discussed so far?
>
> Thanks,
> Julien
>

No problem. I will finish v2 patch as soon as possible.

The mail server seems to have rejected my last mail, which is not
displayed in the mail list. I hope this email can be displayed normally.


Thanks,
tiantian




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Thu, 01 Sep 2022 03:54:01 GMT) Full text and rfc822 format available.

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

From: tiantian <typ22 <at> foxmail.com>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 0/2] Add support for chain-loader
Date: Thu, 01 Sep 2022 11:51:51 +0800
Hi,

Maxime Devos <maximedevos <at> telenet.be> writes:

> [[PGP Signed Part:Undecided]]
>> Why do I want to use chainloader instead of linux and initrd?
>>
>> 1. guix system and archlinux are independent of each other in my computer.
>> They are just on the same computer, but they have their own disk partitions.
>> I also don't want them to interfere with each other.
>> (I have no money to buy more computers. I just have one computer.)
> Sounds a practical system to me.
>
> On 31-08-2022 07:23, typ22 <at> foxmail.com wrote:
>> Is the chain-loader free?
>>
>> The chain-loader is a function of grub, and grub's protocol is GPL3.
>> I think it is free. Although chain-loader can boot nonfree system like windows,
>> it's like nonfree software can run on linux-libre, and linux-libre is free.
>>
>> If Guix think it's nonfree, I will only keep these changes in my computer.
>
> I do believe it counts as free, especially given that it can be used
> to boot into other free systems (e.g., Trisquel) and that you example
> usage is for booting into a largely-free system.
>
> Greetings,
> Maxime.
>
> [2. OpenPGP public key --- application/pgp-keys; OpenPGP_0x49E3EE22191725EE.asc]...
>
> [[End of PGP Signed Part]]

Thank you for your support.

thanks,
tiantian




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Thu, 01 Sep 2022 06:08:01 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: Julien Lepiller <julien <at> lepiller.eu>, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v2 1/2] gnu: bootloader: Extend `<menu-entry>' for
 chain-loader.
Date: Thu,  1 Sep 2022 14:03:14 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
* doc/guix.texi (Bootloader Configuration): Document it.
---

Correct the document and specify all fields of <menu-entry> in the pattern
of menu-entry->sexp.

Now still judge linux, multiboot-kernel and chain-loader. Because if don't
judge, the menu-entry that all of linux, linux-multiboot and chainloader
are #f will match the first one.

Then produce Error message:
guix system: error: #<unspecified>: invalid G-expression input

This error message does not help.

 doc/guix.texi      | 18 ++++++++++++++++++
 gnu/bootloader.scm | 34 ++++++++++++++++++++++++++++------
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 957b9a668e..3f0317ba60 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -37536,6 +37536,24 @@ Bootloader Configuration
              @dots{}))
 @end lisp
 
+@item @code{chain-loader} (default: @code{#f})
+A string that can be accepted by @code{grub}'s @code{chainloader}
+directive. This has no effect if either @code{linux} or
+@code{linux-multiboot} fields are specified. The following is an
+example of chainloading a different GNU/Linux system.
+
+@lisp
+(bootloader
+ (bootloader-configuration
+  ;; @dots{}
+  (menu-entries
+   (list
+    (menu-entry
+     (label "GNU/Linux")
+     (device (uuid "1C31-A17C" 'fat))
+     (chain-loader "/EFI/GNULinux/grubx64.efi"))))))
+@end lisp
+
 @end table
 @end deftp
 
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 77c05e8946..03c18ad0c9 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
             menu-entry-multiboot-kernel
             menu-entry-multiboot-arguments
             menu-entry-multiboot-modules
+            menu-entry-chain-loader
 
             menu-entry->sexp
             sexp->menu-entry
@@ -104,8 +105,11 @@ (define-record-type* <menu-entry>
   (multiboot-arguments menu-entry-multiboot-arguments
                        (default '()))      ; list of string-valued gexps
   (multiboot-modules menu-entry-multiboot-modules
-                     (default '())))       ; list of multiboot commands, where
+                     (default '()))        ; list of multiboot commands, where
                                            ; a command is a list of <string>
+  (chain-loader     menu-entry-chain-loader
+                    (default #f)))         ; string, path of efi file
+
 
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
@@ -117,8 +121,9 @@ (define (menu-entry->sexp entry)
        `(label ,(file-system-label->string label)))
       (_ device)))
   (match entry
-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
-                     ())
+    (($ <menu-entry> label device mount-point
+                     (? identity linux) linux-arguments initrd
+                     #f () () #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
@@ -127,14 +132,22 @@ (define (menu-entry->sexp entry)
                   (linux-arguments ,linux-arguments)
                   (initrd ,initrd)))
     (($ <menu-entry> label device mount-point #f () #f
-                     multiboot-kernel multiboot-arguments multiboot-modules)
+                     (? identity multiboot-kernel) multiboot-arguments
+                     multiboot-modules #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
                   (multiboot-kernel ,multiboot-kernel)
                   (multiboot-arguments ,multiboot-arguments)
-                  (multiboot-modules ,multiboot-modules)))))
+                  (multiboot-modules ,multiboot-modules)))
+    (($ <menu-entry> label device mount-point #f () #f #f () ()
+                     (? identity chain-loader))
+     `(menu-entry (version 0)
+                  (label ,label)
+                  (device ,(device->sexp device))
+                  (device-mount-point ,mount-point)
+                  (chain-loader ,chain-loader)))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
@@ -171,7 +184,16 @@ (define (sexp->menu-entry sexp)
       (device-mount-point mount-point)
       (multiboot-kernel multiboot-kernel)
       (multiboot-arguments multiboot-arguments)
-      (multiboot-modules multiboot-modules)))))
+      (multiboot-modules multiboot-modules)))
+    (('menu-entry ('version 0)
+                  ('label label) ('device device)
+                  ('device-mount-point mount-point)
+                  ('chain-loader chain-loader) _ ...)
+     (menu-entry
+      (label label)
+      (device (sexp->device device))
+      (device-mount-point mount-point)
+      (chain-loader chain-loader)))))
 
 
 ;;;

base-commit: 6beadc82df204f315d06ea35f2e232bb32f8e440
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Thu, 01 Sep 2022 06:09:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: Julien Lepiller <julien <at> lepiller.eu>, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v2 2/2] gnu: bootloader: grub: Add support for chain-loader.
Date: Thu,  1 Sep 2022 14:03:16 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader/grub.scm (grub-configuration-file): Add support for
chain-loader.
---

It is no different from v1 patch.

 gnu/bootloader/grub.scm | 73 ++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 4f18c9b518..7283257354 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -374,44 +374,57 @@ (define* (grub-configuration-file config entries
     (let ((label (menu-entry-label entry))
           (linux (menu-entry-linux entry))
           (device (menu-entry-device entry))
-          (device-mount-point (menu-entry-device-mount-point entry)))
-      (if linux
-          (let ((arguments (menu-entry-linux-arguments entry))
-                (linux (normalize-file linux
-                                       device-mount-point
-                                       store-directory-prefix))
-                (initrd (normalize-file (menu-entry-initrd entry)
-                                        device-mount-point
-                                        store-directory-prefix)))
-         ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
-         ;; Use the right file names for LINUX 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 linux and
-         ;; initrd paths, to allow booting from a Btrfs subvolume.
-         #~(format port "menuentry ~s {
+          (device-mount-point (menu-entry-device-mount-point entry))
+          (multiboot-kernel (menu-entry-multiboot-kernel entry))
+          (chain-loader (menu-entry-chain-loader entry)))
+      (cond
+       (linux
+        (let ((arguments (menu-entry-linux-arguments entry))
+              (linux (normalize-file linux
+                                     device-mount-point
+                                     store-directory-prefix))
+              (initrd (normalize-file (menu-entry-initrd entry)
+                                      device-mount-point
+                                      store-directory-prefix)))
+          ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
+          ;; Use the right file names for LINUX 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 linux and
+          ;; initrd paths, to allow booting from a Btrfs subvolume.
+          #~(format port "menuentry ~s {
   ~a
   linux ~a ~a
   initrd ~a
 }~%"
-                   #$label
-                   #$(grub-root-search device linux)
-                   #$linux (string-join (list #$@arguments))
-                   #$initrd))
-          (let ((kernel (menu-entry-multiboot-kernel entry))
-                (arguments (menu-entry-multiboot-arguments entry))
-                (modules (menu-entry-multiboot-modules entry))
-                (root-index 1))            ; XXX EFI will need root-index 2
-        #~(format port "
+                    #$label
+                    #$(grub-root-search device linux)
+                    #$linux (string-join (list #$@arguments))
+                    #$initrd)))
+       (multiboot-kernel
+        (let ((kernel (menu-entry-multiboot-kernel entry))
+              (arguments (menu-entry-multiboot-arguments entry))
+              (modules (menu-entry-multiboot-modules entry))
+              (root-index 1))            ; XXX EFI will need root-index 2
+          #~(format port "
 menuentry ~s {
   multiboot ~a root=device:hd0s~a~a~a
+}~%"
+                    #$label
+                    #$kernel
+                    #$root-index (string-join (list #$@arguments) " " 'prefix)
+                    (string-join (map string-join '#$modules)
+                                 "\n  module " 'prefix))))
+       (chain-loader
+        #~(format port "
+menuentry ~s {
+  ~a
+  chainloader ~a
 }~%"
                   #$label
-                  #$kernel
-                  #$root-index (string-join (list #$@arguments) " " 'prefix)
-                  (string-join (map string-join '#$modules)
-                               "\n  module " 'prefix))))))
+                  #$(grub-root-search device chain-loader)
+                  #$chain-loader)))))
 
   (define (crypto-devices)
     (define (crypto-device->cryptomount dev)
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Fri, 02 Sep 2022 03:38:01 GMT) Full text and rfc822 format available.

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

From: tiantian <typ22 <at> foxmail.com>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>'
 for chain-loader.
Date: Fri, 02 Sep 2022 09:04:15 +0800
Hi Lepiller,

>>
>> Let's try something like this:
>>
>> @item @code{chain-loader} (default: @code{#f})
>> A string that can be accepted by @code{grub}'s @code{chainloader}
>> directive. This has no effect if either @code{linux} or
>> @code{linux-multiboot} fields are specified. The following is an
>> example of chainloading a different GNU/Linux system.
>>
>
> Thank you for your help. I will change it in next patch.
>
> But I have a little doubt. 'linux-multiboot' has never appeared in the
> documentation. Will it be difficult to understand the document? I don't
> know much about multiboot. I haven't seen the "linux-" prefix in
> multiboot before. Does multiboot only support linux?
>

After reviewing some documents of grub. How about changing
"linux-multiboot" to "multiboot" or "multiboot-kernel" in the document?
The first is because multiboot is used in grub manual. The scecond is
because multiboot-kernel is a field that appears in guix manual.

like this:

@item @code{chain-loader} (default: @code{#f})
A string that can be accepted by @code{grub}'s @code{chainloader}
directive. This has no effect if either @code{linux} or
@code{multiboot} fields are specified. The following is an
example of chainloading a different GNU/Linux system.

or

@item @code{chain-loader} (default: @code{#f})
A string that can be accepted by @code{grub}'s @code{chainloader}
directive. This has no effect if either @code{linux} or
@code{multiboot-kernel} fields are specified. The following is an
example of chainloading a different GNU/Linux system.

>>
>> I prefer this variant where the pattern is explicit.
>>
>> As with what we have today, if the user specifies more than one of
>> linux, linux-multiboot and chainloader, they get an unhelpful "no
>> matching pattern" error.
>>
>> This could be done later if you don't have time, but I would suggest to
>> fix it by adding a default case that matches all incorrect cases, like
>> so:
>>
>> (_ (raise (condition (&message (message (G_ "Your error message
>> here"))))))
>>
>> Have a look at other "&message" conditions for inspiration.
>>
>> Also I noticed that if all of linux, linux-multiboot and chainloader
>> are #f, then the first pattern matches and will lead to a different
>> error message. I haven't tested so I'm not sure what we get, but it
>> might be interresting to match on all of them being #f, and print a
>> different message. Again, this can be done later.
>>
>
> Thank you for your suggestions. I will use in the pattern to specify all
> fields of <menu-entry> in next patch.
> I didn't know how to throw an error message before. I may need to spend
> time reading code and learning. If possible, I will implement it in v3
> patch.
>

You should be right.

There are many situations that I need to check. I can't find a case in
menu-entry->sexp to solve it. So, I wrote a function alone. After I have
preliminarily completed the function that checks menu-entry, I found
that it seems that the explicit pattern is more readable than my
individual function.

The procedure that check menu-entry will check that there is no boot,
different fields of boot are mixed, and there are multiple boot modes. I
haven't tested it yet. The expected effect is as follows.

--- start examples ----

(menu-entry
 (label "example")
 (linux "...")
 (initrd "..."))

Pass check, and no error messages are reported.

(menu-entry
 (label "example")
 (linux #f)
 (initrd #f))

(menu-entry
 (label "example")
 (linux "...")
 (initrd #f))

Becase initrd is required, they report the same error message as
following.

Please select one of following.
1. boot directly (linux + linux-arguments + linux-modules)
2. multiboot (multiboot-kernel + multiboot-arguments + multiboot-modules)
3. chain-loader(chain-loader)

(menu-entry
 (label "example")
 (linux "...")
 (linux-arguments '(...))
 (initrd "...")
 (chain-loader "..."))

Becase It is complete for boot directly by linux and complete for
chainloader, reporting the message as following.

Please don't have more than one boot methods
1. boot directly (linux + linux-arguments + linux-modules)
2. multiboot (multiboot-kernel + multiboot-arguments + multiboot-modules)
3. chain-loader(chain-loader)

(menu-entry
 (label "example")
 (linux "...")
 (initrd "...")
 (multiboot-kernel "..."))

Becase multiboot-kernel shouldn't appear in the boot mode of linux and
the boot mode of multiboot is incomplete, reporting the message as
following.

Please don't mix them.
1. boot directly (linux + linux-arguments + linux-modules)
2. multiboot (multiboot-kernel + multiboot-arguments + multiboot-modules)
3. chain-loader(chain-loader)

--- end examples ---

But the procedure lead more difficult to read and understand the
code. Maybe it's because my code level is not enough. For the current
code, I'm not sure if the error message is worth the performance it
consumes and the code that becomes difficult to understand. The check of
the procedure is not strong. It only checks whether some fields are set,
but not whether the contents of the fields are correct.

And I think the most important point is that the procedure just check
menu-entry. menu-entry->sexp still need to check linux, multiboot and
chain-loader. if not check, An incorrect match will occur in
menu-entry->sexp, and an error message that is not helpful may be
reported by 'guix system'.

I think it might be good to use the menu-entry->sexp in v2 patch. Should
I keep menu-entry->sexp of v2 in v3 patch, in addition to adding some
necessary comments?

The code of the procedure is following.

--- >8 ---

(define (check-menu-entry entry)
  (define (all-correct? fields)
    "Returns a pair according to the number of #t in the FIELDS.
If all of the FIELDS are #t, the pair is (#t . #t). If the part of FIELDS
is #t, the pair is (#t . #t). If all of the FIELDS aren't #t, the pair is
(#f . #f)."
    (let ((total (length fields))
          (right (length (filter identity
                                 fields))))
      (cond ((= right 0) (cons #f #f))
            ((< right total) (cons #t #f))
            ((= right total) (cons #t #t)))))
  (define (get-all-correct-amount boot-methods right-number)
    (match boot-methods
      (((_ . #t) rest ...)
       (get-all-correct-amount rest (1+ right-number)))
      (((_ . #f) rest ...)
       (get-all-correct-amount rest right-number))
      (() right-number)))
  (define (get-part-correct-amount boot-methods number)
    (match boot-methods
      (((_ . #t) rest ...)
       (get-part-correct-amount rest number))
      (((#t . #f) rest ...)
       (get-part-correct-amount rest (1+ number)))
      (((#f . #f) rest ...)
       (get-part-correct-amount rest number))
      (() number)))
  (match-record entry <menu-entry>
    (label
     linux initrd multiboot-kernel multiboot-arguments
     multiboot-modules chain-loader)
    (let* ((linux-boot?
            (all-correct? (list linux initrd)))
           (multiboot?
            (all-correct?
             (list multiboot-kernel
                   (not (null? multiboot-arguments))
                   (not (null? multiboot-modules)))))
           (chain-loader?
            (all-correct? (list chain-loader)))
           (boot-method-all-amount
            (get-all-correct-amount
             (list linux-boot?
                   multiboot?
                   chain-loader?)
             0))
           (boot-method-part-amount
            (get-part-correct-amount
             (list linux-boot?
                   multiboot?
                   chain-loader?) 0))
           (selects "
1. boot directly (linux + linux-arguments + linux-modules)
2. multiboot (multiboot-kernel + multiboot-arguments + multiboot-modules)
3. chain-loader(chain-loader)\n")
           ((raise-message
             (lambda (message)
               (raise (condition
                       (&message
                        (format #f
                         (G_ "invalid menu-entry: ~a~%~a~%~a~%")
                         entry message selects))))))))
      (match (list boot-method-part-amount boot-method-all-amount)
             ((0 0)
              (raise-message "Please select one of following."))
             ((0 1) #t)
             ((0 (? (lambda (n) (> n 1)) _))
              (raise-message "Plase don't have more than one boot method."))
             (((? (lambda (n) (> n 0)) _) _)
              (raise-message "Plese don't mix them."))))))

--- 8< ---

Without any exceptions, v3 patch may be the last version. So how can I
modify the submission information to record your help to me?

I would like to express my sincere thanks to you.I look forward to your
reply.

Thanks,
tiantian




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sat, 03 Sep 2022 20:09:01 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: tiantian <typ22 <at> foxmail.com>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>'
 for chain-loader.
Date: Sat, 3 Sep 2022 22:08:35 +0200
Hi tiantian,

Le Fri, 02 Sep 2022 09:04:15 +0800,
tiantian <typ22 <at> foxmail.com> a écrit :

> Hi Lepiller,

Please call me Julien, I'm more used to being called by my first name.

> 
> After reviewing some documents of grub. How about changing
> "linux-multiboot" to "multiboot" or "multiboot-kernel" in the
> document? The first is because multiboot is used in grub manual. The
> scecond is because multiboot-kernel is a field that appears in guix
> manual.
> 
> like this:
> 
> @item @code{chain-loader} (default: @code{#f})
> A string that can be accepted by @code{grub}'s @code{chainloader}
> directive. This has no effect if either @code{linux} or
> @code{multiboot} fields are specified. The following is an
> example of chainloading a different GNU/Linux system.
> 
> or
> 
> @item @code{chain-loader} (default: @code{#f})
> A string that can be accepted by @code{grub}'s @code{chainloader}
> directive. This has no effect if either @code{linux} or
> @code{multiboot-kernel} fields are specified. The following is an
> example of chainloading a different GNU/Linux system.

You're right, I made a mistake here, I meant the multiboot-kernel
field, like that second example.


I have no problem pushing the patches you have so far without any error
message, although I can still see one issue. In the grub manual I see
this example of chain-loading:

menuentry "Windows" {
	insmod chain
	insmod ntfs
	set root=(hd0,1)
	chainloader +1
}

which I cannot reproduce. I've used "chainloader +1" (by writing my own
grub.cfg manually) with Haiku which is another free OS (though I think
it uses nonfree blobs for hardware), so I think we should support this
use case too, not just booting another EFI entry. When I set
chain-loader "+1", I get the following grub config:

menuentry "haiku" { 

  chainloader +1
}

With no root. I think this is because of our grub-root-search
procedure, which doesn't work in that case.

Do you have any ideas how to support that use-case? If it's too complex
I'm fine leaving it behind for now. It should not slow us down.

> 
> There are many situations that I need to check. I can't find a case in
> menu-entry->sexp to solve it. So, I wrote a function alone. After I
> have preliminarily completed the function that checks menu-entry, I
> found that it seems that the explicit pattern is more readable than my
> individual function.
> 
> The procedure that check menu-entry will check that there is no boot,
> different fields of boot are mixed, and there are multiple boot
> modes. I haven't tested it yet. The expected effect is as follows.
> 
> --- start examples ----
> 

Those examples are nice :)

> 
> --- end examples ---
> 
> But the procedure lead more difficult to read and understand the
> code. Maybe it's because my code level is not enough. For the current
> code, I'm not sure if the error message is worth the performance it
> consumes and the code that becomes difficult to understand. The check
> of the procedure is not strong. It only checks whether some fields
> are set, but not whether the contents of the fields are correct.
> 
> And I think the most important point is that the procedure just check
> menu-entry. menu-entry->sexp still need to check linux, multiboot and
> chain-loader. if not check, An incorrect match will occur in
> menu-entry->sexp, and an error message that is not helpful may be
> reported by 'guix system'.
> 
> I think it might be good to use the menu-entry->sexp in v2 patch.
> Should I keep menu-entry->sexp of v2 in v3 patch, in addition to
> adding some necessary comments?

Let's keep the code from v2.

> 
> The code of the procedure is following.
> 
> --- >8 ---  
> 
> (define (check-menu-entry entry)
>   (define (all-correct? fields)
>     "Returns a pair according to the number of #t in the FIELDS.
> If all of the FIELDS are #t, the pair is (#t . #t). If the part of
> FIELDS is #t, the pair is (#t . #t). If all of the FIELDS aren't #t,
> the pair is (#f . #f)."
>     (let ((total (length fields))
>           (right (length (filter identity
>                                  fields))))
>       (cond ((= right 0) (cons #f #f))
>             ((< right total) (cons #t #f))
>             ((= right total) (cons #t #t)))))
>   (define (get-all-correct-amount boot-methods right-number)
>     (match boot-methods
>       (((_ . #t) rest ...)
>        (get-all-correct-amount rest (1+ right-number)))
>       (((_ . #f) rest ...)
>        (get-all-correct-amount rest right-number))
>       (() right-number)))
>   (define (get-part-correct-amount boot-methods number)
>     (match boot-methods
>       (((_ . #t) rest ...)
>        (get-part-correct-amount rest number))
>       (((#t . #f) rest ...)
>        (get-part-correct-amount rest (1+ number)))
>       (((#f . #f) rest ...)
>        (get-part-correct-amount rest number))
>       (() number)))
>   (match-record entry <menu-entry>
>     (label
>      linux initrd multiboot-kernel multiboot-arguments
>      multiboot-modules chain-loader)
>     (let* ((linux-boot?
>             (all-correct? (list linux initrd)))
>            (multiboot?
>             (all-correct?
>              (list multiboot-kernel
>                    (not (null? multiboot-arguments))
>                    (not (null? multiboot-modules)))))
>            (chain-loader?
>             (all-correct? (list chain-loader)))
>            (boot-method-all-amount
>             (get-all-correct-amount
>              (list linux-boot?
>                    multiboot?
>                    chain-loader?)
>              0))
>            (boot-method-part-amount
>             (get-part-correct-amount
>              (list linux-boot?
>                    multiboot?
>                    chain-loader?) 0))
>            (selects "
> 1. boot directly (linux + linux-arguments + linux-modules)
> 2. multiboot (multiboot-kernel + multiboot-arguments +
> multiboot-modules) 3. chain-loader(chain-loader)\n")
>            ((raise-message
>              (lambda (message)
>                (raise (condition
>                        (&message
>                         (format #f
>                          (G_ "invalid menu-entry: ~a~%~a~%~a~%")
>                          entry message selects))))))))
>       (match (list boot-method-part-amount boot-method-all-amount)
>              ((0 0)
>               (raise-message "Please select one of following."))
>              ((0 1) #t)
>              ((0 (? (lambda (n) (> n 1)) _))
>               (raise-message "Plase don't have more than one boot
> method.")) (((? (lambda (n) (> n 0)) _) _)
>               (raise-message "Plese don't mix them."))))))
> 
> --- 8< ---

Maybe it would be easier to have something like this:

(define (check-menu-entry menu-entry)
  (define all-linux-fields
    (and (menu-entry-linux menu-entry)
         (menu-entry-linux-arguments menu-entry)
         (menu-entry-linux-modules menu-entry)))
  (define all-multiboot-fields
    ...)
  (define all-chainload-fields
    ...)
  (define count-methods
    (+ (if all-linux-fields 1 0)
       (if all-multiboot-fields 1 0)
       (if all-chainload-fields 1 0)))

  (define (report err)
    (raise
      (condition
        (&message
          (message
            (format #f (G_ "invalid menu-entry: ~a" err))))
        (&fix-hint
          (hint
            (G_ "Please chose only one of:
@enumerate
@item direct boot by specifying fields @code{linux},
@code{linux-arguments} and @code{linux-modules},
@item multiboot by specifying fields @code{multiboot-kernel},
@code{multiboot-arguments} and @code{multiboot-modules},
@item chain-loader by specifying field @code{chain-loader}.
@end enumerate"))))))

  (cond
    ((and (not all-linux-fields) (not all-multiboot-fields)
          (not all-chainload-fields))
     (report (G_ "No boot method was chosen for this menu entry.")))
    ((> count-methods 1)
     (report (G_ "More than two boot methods were selected for this
menu entry.")))
    (else #t)))

This is untested, so there might be bugs :)

Note that we need to have all strings translatable (with G_).

In any case, that new code for error messages should go in a third,
separate patch.

> 
> Without any exceptions, v3 patch may be the last version. So how can I
> modify the submission information to record your help to me?

You don't have to because I didn't contribute much to that patch, and I
will sign it off when commiting. If you take the code above as is and
don't modify it too much, then you can add something like this at the
end of the commit message (only for the patch that contains my code):

Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>

> 
> I would like to express my sincere thanks to you.I look forward to
> your reply.
> 
> Thanks,
> tiantian





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 13:55:02 GMT) Full text and rfc822 format available.

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

From: tiantian <typ22 <at> foxmail.com>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>'
 for chain-loader.
Date: Sun, 04 Sep 2022 21:09:35 +0800
Hi Julien,

Julien Lepiller <julien <at> lepiller.eu> writes:

> Hi tiantian,
>
> Le Fri, 02 Sep 2022 09:04:15 +0800,
> tiantian <typ22 <at> foxmail.com> a écrit :
>
>> Hi Lepiller,
>
> Please call me Julien, I'm more used to being called by my first name.
>

Ok. If I have offended you before, please forgive me.

>
> You're right, I made a mistake here, I meant the multiboot-kernel
> field, like that second example.
>

OK, I'll write the second one in the document.

>
> I have no problem pushing the patches you have so far without any error
> message, although I can still see one issue. In the grub manual I see
> this example of chain-loading:
>
> menuentry "Windows" {
> 	insmod chain
> 	insmod ntfs
> 	set root=(hd0,1)
> 	chainloader +1
> }
>
> which I cannot reproduce. I've used "chainloader +1" (by writing my own
> grub.cfg manually) with Haiku which is another free OS (though I think
> it uses nonfree blobs for hardware), so I think we should support this
> use case too, not just booting another EFI entry. When I set
> chain-loader "+1", I get the following grub config:
>
> menuentry "haiku" { 
>
>   chainloader +1
> }
>
> With no root. I think this is because of our grub-root-search
> procedure, which doesn't work in that case.
>
> Do you have any ideas how to support that use-case? If it's too complex
> I'm fine leaving it behind for now. It should not slow us down.
>

For the present `grub-root-search', I should not try to modify it for the
time being. Because the device field may be associated with the
boot-parameters, and modifying it may require more work.

I have never used 'chainloader +1'. But if you just want to specify root
device by the form like '(hd0,1)', you can try like this:

(chain-loader "(hd0,1)+1")

You may get some help from this article.
(https://blog.cykerway.com/posts/2016/12/27/grub-chainloader-1.html)

You may also get some help from "16.3.10 chainloader" and "13.3 How to
specify block lists" in grub manual.

I can't test these on my computer because I don't know how to use them.
I hope these will help you.

>> 
>> There are many situations that I need to check. I can't find a case in
>> menu-entry->sexp to solve it. So, I wrote a function alone. After I
>> have preliminarily completed the function that checks menu-entry, I
>> found that it seems that the explicit pattern is more readable than my
>> individual function.
>> 
>> The procedure that check menu-entry will check that there is no boot,
>> different fields of boot are mixed, and there are multiple boot
>> modes. I haven't tested it yet. The expected effect is as follows.
>> 
>> --- start examples ----
>> 
>
> Those examples are nice :)
>

Thank you for your praise.

>> 
>> --- end examples ---
>> 
>> But the procedure lead more difficult to read and understand the
>> code. Maybe it's because my code level is not enough. For the current
>> code, I'm not sure if the error message is worth the performance it
>> consumes and the code that becomes difficult to understand. The check
>> of the procedure is not strong. It only checks whether some fields
>> are set, but not whether the contents of the fields are correct.
>> 
>> And I think the most important point is that the procedure just check
>> menu-entry. menu-entry->sexp still need to check linux, multiboot and
>> chain-loader. if not check, An incorrect match will occur in
>> menu-entry->sexp, and an error message that is not helpful may be
>> reported by 'guix system'.
>> 
>> I think it might be good to use the menu-entry->sexp in v2 patch.
>> Should I keep menu-entry->sexp of v2 in v3 patch, in addition to
>> adding some necessary comments?
>
> Let's keep the code from v2.
>

I thought so before, but your code has brought me new
inspiration. Perhaps, it will have some fine-tuning.

>
> Maybe it would be easier to have something like this:
>
> (define (check-menu-entry menu-entry)
>   (define all-linux-fields
>     (and (menu-entry-linux menu-entry)
>          (menu-entry-linux-arguments menu-entry)
>          (menu-entry-linux-modules menu-entry)))
>   (define all-multiboot-fields
>     ...)
>   (define all-chainload-fields
>     ...)
>   (define count-methods
>     (+ (if all-linux-fields 1 0)
>        (if all-multiboot-fields 1 0)
>        (if all-chainload-fields 1 0)))
>
>   (define (report err)
>     (raise
>       (condition
>         (&message
>           (message
>             (format #f (G_ "invalid menu-entry: ~a" err))))
>         (&fix-hint
>           (hint
>             (G_ "Please chose only one of:
> @enumerate
> @item direct boot by specifying fields @code{linux},
> @code{linux-arguments} and @code{linux-modules},
> @item multiboot by specifying fields @code{multiboot-kernel},
> @code{multiboot-arguments} and @code{multiboot-modules},
> @item chain-loader by specifying field @code{chain-loader}.
> @end enumerate"))))))
>
>   (cond
>     ((and (not all-linux-fields) (not all-multiboot-fields)
>           (not all-chainload-fields))
>      (report (G_ "No boot method was chosen for this menu entry.")))
>     ((> count-methods 1)
>      (report (G_ "More than two boot methods were selected for this
> menu entry.")))
>     (else #t)))
>
> This is untested, so there might be bugs :)
>
> Note that we need to have all strings translatable (with G_).
>
> In any case, that new code for error messages should go in a third,
> separate patch.
>

Thanks your help. When I first add and test your check-menu-entry, I was
amazed by its good-looking error reporting information. I like the error
message.

But I still have to work out the situation that linux,linux-argumet and
initrd are set, and multiboot-kernel are also set, which can pass the
check, but it will fail to match in menu-entry->sexp. Yes,
multiboot-kernel shouldn't be ignored silently. But error message of no
match is simple and crude. And It is also inconsistent with the previous
exquisite error reporting information.

When thinking about how to solve this problem, I get inspiration from
it. It correct my previous wrong ideas. Mistakes are always various,
and it is difficult for us to consider all of them. But the correctness
is certain, and we can consider it more easily. I should not try to
cover all the error cases, because it is very difficult. Even if it
succeeds, the code is too complex and difficult to read. However, as
long as I rule out the right situations, the rest is the wrong
situation. "There are many situations that I need to check. I can't find
a case in menu-entry->sexp to solve it." It is caused by my wrong
thinking direction.

So I split `check-menu-entry', merge the checked part intoe the pattern
of `menu-entry->sexp', and separate the function of reporting error
information into a procedure `report menu entry error'. Then add 'else'
to the pattern in `menu-entry->sexp' to report error message.

About the error message,I give up to point out what might be wrong, but
display `menu-entry' and hint message.

There are two main reasons:

1. As mentioned above, errors are unpredictable, and wrong error
reporting information may mislead users.

2. The error is not caused by `menu-entry->sexp', but by an external
procedure. This needs to display the `menu-entry' accepted by
`menu-entry->sexp' to let the user know.

Thank you for your code's inspiration. And your hint information is
really great. I use your code about error message directly.

>> 
>> Without any exceptions, v3 patch may be the last version. So how can I
>> modify the submission information to record your help to me?
>
> You don't have to because I didn't contribute much to that patch, and I
> will sign it off when commiting. If you take the code above as is and
> don't modify it too much, then you can add something like this at the
> end of the commit message (only for the patch that contains my code):
>
> Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
>

No, you give me great help. you correct the document, give me advice of
using the pattern, and take time to find bug of my code and give me some
advice how to work out it.

Thanks your help. I will add "Co-Authored-By: Julien Lepiller
<julien <at> lepiller.eu>" to commit messages of my first and third patch.

Thanks,
tiantian




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 14:12:01 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v3 1/3] gnu: bootloader: Extend `<menu-entry>' for
 chain-loader.
Date: Sun,  4 Sep 2022 22:04:26 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
* doc/guix.texi (Bootloader Configuration): Document it.

Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
---
v2:
Correct the document and specify all fields of <menu-entry> in the pattern
of menu-entry->sexp.

Now still judge linux, multiboot-kernel and chain-loader. Because if don't
judge, the menu-entry that all of linux, linux-multiboot and chainloader
are #f will match the first one.

Then produce Error message:
guix system: error: #<unspecified>: invalid G-expression input

This error message does not help.

v3:
correct the document and delete an extra blank line.

all:
The documents are mainly corrected by Julien Lepiller.
Thank you here.

 doc/guix.texi      | 18 ++++++++++++++++++
 gnu/bootloader.scm | 33 +++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 957b9a668e..cc64a7ed70 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -37536,6 +37536,24 @@ Bootloader Configuration
              @dots{}))
 @end lisp
 
+@item @code{chain-loader} (default: @code{#f})
+A string that can be accepted by @code{grub}'s @code{chainloader}
+directive. This has no effect if either @code{linux} or
+@code{multiboot-kernel} fields are specified. The following is an
+example of chainloading a different GNU/Linux system.
+
+@lisp
+(bootloader
+ (bootloader-configuration
+  ;; @dots{}
+  (menu-entries
+   (list
+    (menu-entry
+     (label "GNU/Linux")
+     (device (uuid "1C31-A17C" 'fat))
+     (chain-loader "/EFI/GNULinux/grubx64.efi"))))))
+@end lisp
+
 @end table
 @end deftp
 
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 77c05e8946..9e8b545d10 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
             menu-entry-multiboot-kernel
             menu-entry-multiboot-arguments
             menu-entry-multiboot-modules
+            menu-entry-chain-loader
 
             menu-entry->sexp
             sexp->menu-entry
@@ -104,8 +105,10 @@ (define-record-type* <menu-entry>
   (multiboot-arguments menu-entry-multiboot-arguments
                        (default '()))      ; list of string-valued gexps
   (multiboot-modules menu-entry-multiboot-modules
-                     (default '())))       ; list of multiboot commands, where
+                     (default '()))        ; list of multiboot commands, where
                                            ; a command is a list of <string>
+  (chain-loader     menu-entry-chain-loader
+                    (default #f)))         ; string, path of efi file
 
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
@@ -117,8 +120,9 @@ (define (menu-entry->sexp entry)
        `(label ,(file-system-label->string label)))
       (_ device)))
   (match entry
-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
-                     ())
+    (($ <menu-entry> label device mount-point
+                     (? identity linux) linux-arguments initrd
+                     #f () () #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
@@ -127,14 +131,22 @@ (define (menu-entry->sexp entry)
                   (linux-arguments ,linux-arguments)
                   (initrd ,initrd)))
     (($ <menu-entry> label device mount-point #f () #f
-                     multiboot-kernel multiboot-arguments multiboot-modules)
+                     (? identity multiboot-kernel) multiboot-arguments
+                     multiboot-modules #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
                   (multiboot-kernel ,multiboot-kernel)
                   (multiboot-arguments ,multiboot-arguments)
-                  (multiboot-modules ,multiboot-modules)))))
+                  (multiboot-modules ,multiboot-modules)))
+    (($ <menu-entry> label device mount-point #f () #f #f () ()
+                     (? identity chain-loader))
+     `(menu-entry (version 0)
+                  (label ,label)
+                  (device ,(device->sexp device))
+                  (device-mount-point ,mount-point)
+                  (chain-loader ,chain-loader)))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
@@ -171,7 +183,16 @@ (define (sexp->menu-entry sexp)
       (device-mount-point mount-point)
       (multiboot-kernel multiboot-kernel)
       (multiboot-arguments multiboot-arguments)
-      (multiboot-modules multiboot-modules)))))
+      (multiboot-modules multiboot-modules)))
+    (('menu-entry ('version 0)
+                  ('label label) ('device device)
+                  ('device-mount-point mount-point)
+                  ('chain-loader chain-loader) _ ...)
+     (menu-entry
+      (label label)
+      (device (sexp->device device))
+      (device-mount-point mount-point)
+      (chain-loader chain-loader)))))
 
 
 ;;;

base-commit: 6beadc82df204f315d06ea35f2e232bb32f8e440
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 14:12:01 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v3 2/3] gnu: bootloader: grub: Add support for chain-loader.
Date: Sun,  4 Sep 2022 22:04:28 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader/grub.scm (grub-configuration-file): Add support for
chain-loader.
---

It is no different from v2 patch.

 gnu/bootloader/grub.scm | 73 ++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 4f18c9b518..7283257354 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -374,44 +374,57 @@ (define* (grub-configuration-file config entries
     (let ((label (menu-entry-label entry))
           (linux (menu-entry-linux entry))
           (device (menu-entry-device entry))
-          (device-mount-point (menu-entry-device-mount-point entry)))
-      (if linux
-          (let ((arguments (menu-entry-linux-arguments entry))
-                (linux (normalize-file linux
-                                       device-mount-point
-                                       store-directory-prefix))
-                (initrd (normalize-file (menu-entry-initrd entry)
-                                        device-mount-point
-                                        store-directory-prefix)))
-         ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
-         ;; Use the right file names for LINUX 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 linux and
-         ;; initrd paths, to allow booting from a Btrfs subvolume.
-         #~(format port "menuentry ~s {
+          (device-mount-point (menu-entry-device-mount-point entry))
+          (multiboot-kernel (menu-entry-multiboot-kernel entry))
+          (chain-loader (menu-entry-chain-loader entry)))
+      (cond
+       (linux
+        (let ((arguments (menu-entry-linux-arguments entry))
+              (linux (normalize-file linux
+                                     device-mount-point
+                                     store-directory-prefix))
+              (initrd (normalize-file (menu-entry-initrd entry)
+                                      device-mount-point
+                                      store-directory-prefix)))
+          ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
+          ;; Use the right file names for LINUX 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 linux and
+          ;; initrd paths, to allow booting from a Btrfs subvolume.
+          #~(format port "menuentry ~s {
   ~a
   linux ~a ~a
   initrd ~a
 }~%"
-                   #$label
-                   #$(grub-root-search device linux)
-                   #$linux (string-join (list #$@arguments))
-                   #$initrd))
-          (let ((kernel (menu-entry-multiboot-kernel entry))
-                (arguments (menu-entry-multiboot-arguments entry))
-                (modules (menu-entry-multiboot-modules entry))
-                (root-index 1))            ; XXX EFI will need root-index 2
-        #~(format port "
+                    #$label
+                    #$(grub-root-search device linux)
+                    #$linux (string-join (list #$@arguments))
+                    #$initrd)))
+       (multiboot-kernel
+        (let ((kernel (menu-entry-multiboot-kernel entry))
+              (arguments (menu-entry-multiboot-arguments entry))
+              (modules (menu-entry-multiboot-modules entry))
+              (root-index 1))            ; XXX EFI will need root-index 2
+          #~(format port "
 menuentry ~s {
   multiboot ~a root=device:hd0s~a~a~a
+}~%"
+                    #$label
+                    #$kernel
+                    #$root-index (string-join (list #$@arguments) " " 'prefix)
+                    (string-join (map string-join '#$modules)
+                                 "\n  module " 'prefix))))
+       (chain-loader
+        #~(format port "
+menuentry ~s {
+  ~a
+  chainloader ~a
 }~%"
                   #$label
-                  #$kernel
-                  #$root-index (string-join (list #$@arguments) " " 'prefix)
-                  (string-join (map string-join '#$modules)
-                               "\n  module " 'prefix))))))
+                  #$(grub-root-search device chain-loader)
+                  #$chain-loader)))))
 
   (define (crypto-devices)
     (define (crypto-device->cryptomount dev)
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 14:14:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Sun,  4 Sep 2022 22:04:31 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (report-menu-entry-error): New procedure.
(menu-entry->sexp): Modify the pattern and add `else' to call
`report-menu-entry-error'.

Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
---
The error message like so:

guix system: error: invalid menu-entry: #<<menu-entry> label: "test" device: #<file-system-label "guix-root"> device-mount-point: #f linux: #f linux-arguments: () initrd: #f multiboot-kernel: #f multiboot-arguments: () multiboot-modules: () chain-loader: #f>
hint: Please chose only one of:

  1. direct boot by specifying fields `linux', `linux-arguments' and `linux-modules',

  2. multiboot by specifying fields `multiboot-kernel', `multiboot-arguments' and `multiboot-modules',

  3. chain-loader by specifying field `chain-loader'.

The code of `report-menu-entry-error' is quoted from Julien Lepiller.
Thanks the help from Julien Lepiller.

 gnu/bootloader.scm | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 9e8b545d10..d4fe460570 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -34,6 +34,8 @@ (define-module (gnu bootloader)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:export (menu-entry
             menu-entry?
@@ -110,6 +112,23 @@ (define-record-type* <menu-entry>
   (chain-loader     menu-entry-chain-loader
                     (default #f)))         ; string, path of efi file
 
+(define (report-menu-entry-error menu-entry)
+  (raise
+   (condition
+    (&message
+     (message
+      (format #f (G_ "invalid menu-entry: ~a") menu-entry)))
+    (&fix-hint
+     (hint
+      (G_ "Please chose only one of:
+@enumerate
+@item direct boot by specifying fields @code{linux},
+@code{linux-arguments} and @code{linux-modules},
+@item multiboot by specifying fields @code{multiboot-kernel},
+@code{multiboot-arguments} and @code{multiboot-modules},
+@item chain-loader by specifying field @code{chain-loader}.
+@end enumerate"))))))
+
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
   (define (device->sexp device)
@@ -119,9 +138,21 @@ (define (menu-entry->sexp entry)
       ((? file-system-label? label)
        `(label ,(file-system-label->string label)))
       (_ device)))
+  (define (set-field? field)
+    "If the field is the default value, return #f."
+    (and field                  ; not default value #f
+         (not (null? field))))  ; not default value '()
   (match entry
+    ;; Modify the pattern to achieve more strict matching and prevent
+    ;; wrong matching, which ensures the output of error information
+    ;; when menu-entry is wrong.
+
+    ;; The linux-arguments is optional and the test code for boot-parameters
+    ;; does not set it, so don't check it here.
     (($ <menu-entry> label device mount-point
-                     (? identity linux) linux-arguments initrd
+                     (? set-field? linux)
+                     linux-arguments
+                     (? set-field? initrd)
                      #f () () #f)
      `(menu-entry (version 0)
                   (label ,label)
@@ -131,8 +162,10 @@ (define (menu-entry->sexp entry)
                   (linux-arguments ,linux-arguments)
                   (initrd ,initrd)))
     (($ <menu-entry> label device mount-point #f () #f
-                     (? identity multiboot-kernel) multiboot-arguments
-                     multiboot-modules #f)
+                     (? set-field? multiboot-kernel)
+                     (? set-field? multiboot-arguments)
+                     (? set-field? multiboot-modules)
+                     #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
@@ -141,12 +174,13 @@ (define (menu-entry->sexp entry)
                   (multiboot-arguments ,multiboot-arguments)
                   (multiboot-modules ,multiboot-modules)))
     (($ <menu-entry> label device mount-point #f () #f #f () ()
-                     (? identity chain-loader))
+                     (? set-field? chain-loader))
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
-                  (chain-loader ,chain-loader)))))
+                  (chain-loader ,chain-loader)))
+    (else (report-menu-entry-error entry))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 15:39:01 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: typ22 <at> foxmail.com
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Sun, 4 Sep 2022 17:37:55 +0200
Hi tiantian,

I think the first two patches are good now, so let me focus on this one
:)

Le Sun,  4 Sep 2022 22:04:31 +0800,
typ22 <at> foxmail.com a écrit :

> From: tiantian <typ22 <at> foxmail.com>
> 
> +  (define (set-field? field)
> +    "If the field is the default value, return #f."
> +    (and field                  ; not default value #f
> +         (not (null? field))))  ; not default value '()

I don't think this set-field? is necessary. In the following, I don't
think you need the null? case at all because I think all the lists may
be empty without triggering an error. You don't necessarily want to
load modules or have arguments on the linux command line.

In any case, it should be called field-set? instead :)

>    (match entry
> +    ;; Modify the pattern to achieve more strict matching and prevent
> +    ;; wrong matching, which ensures the output of error information
> +    ;; when menu-entry is wrong.
> +
> +    ;; The linux-arguments is optional and the test code for
> boot-parameters
> +    ;; does not set it, so don't check it here.
>      (($ <menu-entry> label device mount-point
> -                     (? identity linux) linux-arguments initrd
> +                     (? set-field? linux)
> +                     linux-arguments
> +                     (? set-field? initrd)

The could still be identity

>                       #f () () #f)
>       `(menu-entry (version 0)
>                    (label ,label)
> @@ -131,8 +162,10 @@ (define (menu-entry->sexp entry)
>                    (linux-arguments ,linux-arguments)
>                    (initrd ,initrd)))
>      (($ <menu-entry> label device mount-point #f () #f
> -                     (? identity multiboot-kernel)
> multiboot-arguments
> -                     multiboot-modules #f)
> +                     (? set-field? multiboot-kernel)
> +                     (? set-field? multiboot-arguments)
> +                     (? set-field? multiboot-modules)

Some users might want to not use any modules or arguments I think, so
these fields should not be mandatory. For multiboot-kernel, identity is
enough.

> +                     #f)
>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
> @@ -141,12 +174,13 @@ (define (menu-entry->sexp entry)
>                    (multiboot-arguments ,multiboot-arguments)
>                    (multiboot-modules ,multiboot-modules)))
>      (($ <menu-entry> label device mount-point #f () #f #f () ()
> -                     (? identity chain-loader))
> +                     (? set-field? chain-loader))

Same here, identity is fine.

>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
>                    (device-mount-point ,mount-point)
> -                  (chain-loader ,chain-loader)))))
> +                  (chain-loader ,chain-loader)))
> +    (else (report-menu-entry-error entry))))

Since this is a match, it is more common to use:

(_ (report-menu-entry-error entry))

Also, it feels weird to patch the code you modified in a previous patch
of the same series. If you're not happy with the code you wrote in a
previous patch, you need to change it in the previous patch, not in a
new one :)

>  
>  (define (sexp->menu-entry sexp)
>    "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a
> <menu-entry>





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 17:18:02 GMT) Full text and rfc822 format available.

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

From: tiantian <typ22 <at> foxmail.com>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Mon, 05 Sep 2022 00:15:12 +0800
Julien Lepiller <julien <at> lepiller.eu> writes:

> Hi tiantian,
>
> I think the first two patches are good now, so let me focus on this one
> :)
>
> Le Sun,  4 Sep 2022 22:04:31 +0800,
> typ22 <at> foxmail.com a écrit :
>
>> From: tiantian <typ22 <at> foxmail.com>
>> 
>> +  (define (set-field? field)
>> +    "If the field is the default value, return #f."
>> +    (and field                  ; not default value #f
>> +         (not (null? field))))  ; not default value '()
>
> I don't think this set-field? is necessary. In the following, I don't
> think you need the null? case at all because I think all the lists may
> be empty without triggering an error. You don't necessarily want to
> load modules or have arguments on the linux command line.
>
> In any case, it should be called field-set? instead :)
>

My English is not good. To be honest, I tried set-value?, value-set?,
default-value?, not-default-value?, field-set? and set-field?. Finally,
I select the 'set-field?'. But it seems that I didn't choose well.

But it doesn't matter. The procedure is no longer needed.

>>    (match entry
>> +    ;; Modify the pattern to achieve more strict matching and prevent
>> +    ;; wrong matching, which ensures the output of error information
>> +    ;; when menu-entry is wrong.
>> +
>> +    ;; The linux-arguments is optional and the test code for
>> boot-parameters
>> +    ;; does not set it, so don't check it here.
>>      (($ <menu-entry> label device mount-point
>> -                     (? identity linux) linux-arguments initrd
>> +                     (? set-field? linux)
>> +                     linux-arguments
>> +                     (? set-field? initrd)
>
> The could still be identity
>
>>                       #f () () #f)
>>       `(menu-entry (version 0)
>>                    (label ,label)
>> @@ -131,8 +162,10 @@ (define (menu-entry->sexp entry)
>>                    (linux-arguments ,linux-arguments)
>>                    (initrd ,initrd)))
>>      (($ <menu-entry> label device mount-point #f () #f
>> -                     (? identity multiboot-kernel)
>> multiboot-arguments
>> -                     multiboot-modules #f)
>> +                     (? set-field? multiboot-kernel)
>> +                     (? set-field? multiboot-arguments)
>> +                     (? set-field? multiboot-modules)
>
> Some users might want to not use any modules or arguments I think, so
> these fields should not be mandatory. For multiboot-kernel, identity is
> enough.
>
>> +                     #f)
>>       `(menu-entry (version 0)
>>                    (label ,label)
>>                    (device ,(device->sexp device))
>> @@ -141,12 +174,13 @@ (define (menu-entry->sexp entry)
>>                    (multiboot-arguments ,multiboot-arguments)
>>                    (multiboot-modules ,multiboot-modules)))
>>      (($ <menu-entry> label device mount-point #f () #f #f () ()
>> -                     (? identity chain-loader))
>> +                     (? set-field? chain-loader))
>
> Same here, identity is fine.
>

I don't know multiboot very well, so I don't know if multiboot-arguments
and multiboot-modules are necessary. Then I check them. It turns out that
they are not necessary. I will not check them.

I will change to use identify. Honestly, It's much easier for me to
use only `identify'. :)

>>       `(menu-entry (version 0)
>>                    (label ,label)
>>                    (device ,(device->sexp device))
>>                    (device-mount-point ,mount-point)
>> -                  (chain-loader ,chain-loader)))))
>> +                  (chain-loader ,chain-loader)))
>> +    (else (report-menu-entry-error entry))))
>
> Since this is a match, it is more common to use:
>
> (_ (report-menu-entry-error entry))
>

Thank you for reminding me. I will correct it.

> Also, it feels weird to patch the code you modified in a previous patch
> of the same series. If you're not happy with the code you wrote in a
> previous patch, you need to change it in the previous patch, not in a
> new one :)
>

As I understood earlier, these changes about matching are related to the
error reporting information, so I put these modifications in this
submission. My knowledge of contribution is still too little.

I will pay attention to it later. Thank you for reminding me.

Thanks,
tiantian




Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 17:27:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v4 1/3] gnu: bootloader: Extend `<menu-entry>' for
 chain-loader.
Date: Mon,  5 Sep 2022 01:25:38 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (<menu-entry>)[chain-loader]: New field.
(menu-entry->sexp, sexp->menu-entry): Support chain-loader.
* doc/guix.texi (Bootloader Configuration): Document it.

Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
---
v2:
Correct the document and specify all fields of <menu-entry> in the pattern
of menu-entry->sexp.

Now still judge linux, multiboot-kernel and chain-loader. Because if don't
judge, the menu-entry that all of linux, linux-multiboot and chainloader
are #f will match the first one.

Then produce Error message:
guix system: error: #<unspecified>: invalid G-expression input

This error message does not help.

v3:
correct the document and delete an extra blank line.

v4:
Add check for initrd in menu-entry->sexp, which is a change moved from the
third patch of v3.

all:
The documents are mainly corrected by Julien Lepiller.
Thank you here.

 doc/guix.texi      | 18 ++++++++++++++++++
 gnu/bootloader.scm | 33 +++++++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 957b9a668e..cc64a7ed70 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -37536,6 +37536,24 @@ Bootloader Configuration
              @dots{}))
 @end lisp
 
+@item @code{chain-loader} (default: @code{#f})
+A string that can be accepted by @code{grub}'s @code{chainloader}
+directive. This has no effect if either @code{linux} or
+@code{multiboot-kernel} fields are specified. The following is an
+example of chainloading a different GNU/Linux system.
+
+@lisp
+(bootloader
+ (bootloader-configuration
+  ;; @dots{}
+  (menu-entries
+   (list
+    (menu-entry
+     (label "GNU/Linux")
+     (device (uuid "1C31-A17C" 'fat))
+     (chain-loader "/EFI/GNULinux/grubx64.efi"))))))
+@end lisp
+
 @end table
 @end deftp
 
diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 77c05e8946..9fe6b65212 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -46,6 +46,7 @@ (define-module (gnu bootloader)
             menu-entry-multiboot-kernel
             menu-entry-multiboot-arguments
             menu-entry-multiboot-modules
+            menu-entry-chain-loader
 
             menu-entry->sexp
             sexp->menu-entry
@@ -104,8 +105,10 @@ (define-record-type* <menu-entry>
   (multiboot-arguments menu-entry-multiboot-arguments
                        (default '()))      ; list of string-valued gexps
   (multiboot-modules menu-entry-multiboot-modules
-                     (default '())))       ; list of multiboot commands, where
+                     (default '()))        ; list of multiboot commands, where
                                            ; a command is a list of <string>
+  (chain-loader     menu-entry-chain-loader
+                    (default #f)))         ; string, path of efi file
 
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
@@ -117,8 +120,9 @@ (define (menu-entry->sexp entry)
        `(label ,(file-system-label->string label)))
       (_ device)))
   (match entry
-    (($ <menu-entry> label device mount-point linux linux-arguments initrd #f
-                     ())
+    (($ <menu-entry> label device mount-point
+                     (? identity linux) linux-arguments (? identity initrd)
+                     #f () () #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
@@ -127,14 +131,22 @@ (define (menu-entry->sexp entry)
                   (linux-arguments ,linux-arguments)
                   (initrd ,initrd)))
     (($ <menu-entry> label device mount-point #f () #f
-                     multiboot-kernel multiboot-arguments multiboot-modules)
+                     (? identity multiboot-kernel) multiboot-arguments
+                     multiboot-modules #f)
      `(menu-entry (version 0)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
                   (multiboot-kernel ,multiboot-kernel)
                   (multiboot-arguments ,multiboot-arguments)
-                  (multiboot-modules ,multiboot-modules)))))
+                  (multiboot-modules ,multiboot-modules)))
+    (($ <menu-entry> label device mount-point #f () #f #f () ()
+                     (? identity chain-loader))
+     `(menu-entry (version 0)
+                  (label ,label)
+                  (device ,(device->sexp device))
+                  (device-mount-point ,mount-point)
+                  (chain-loader ,chain-loader)))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
@@ -171,7 +183,16 @@ (define (sexp->menu-entry sexp)
       (device-mount-point mount-point)
       (multiboot-kernel multiboot-kernel)
       (multiboot-arguments multiboot-arguments)
-      (multiboot-modules multiboot-modules)))))
+      (multiboot-modules multiboot-modules)))
+    (('menu-entry ('version 0)
+                  ('label label) ('device device)
+                  ('device-mount-point mount-point)
+                  ('chain-loader chain-loader) _ ...)
+     (menu-entry
+      (label label)
+      (device (sexp->device device))
+      (device-mount-point mount-point)
+      (chain-loader chain-loader)))))
 
 
 ;;;

base-commit: 6beadc82df204f315d06ea35f2e232bb32f8e440
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 17:28:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v4 2/3] gnu: bootloader: grub: Add support for chain-loader.
Date: Mon,  5 Sep 2022 01:25:40 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader/grub.scm (grub-configuration-file): Add support for
chain-loader.
---

It is no different from v3 patch.

 gnu/bootloader/grub.scm | 73 ++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 4f18c9b518..7283257354 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -374,44 +374,57 @@ (define* (grub-configuration-file config entries
     (let ((label (menu-entry-label entry))
           (linux (menu-entry-linux entry))
           (device (menu-entry-device entry))
-          (device-mount-point (menu-entry-device-mount-point entry)))
-      (if linux
-          (let ((arguments (menu-entry-linux-arguments entry))
-                (linux (normalize-file linux
-                                       device-mount-point
-                                       store-directory-prefix))
-                (initrd (normalize-file (menu-entry-initrd entry)
-                                        device-mount-point
-                                        store-directory-prefix)))
-         ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
-         ;; Use the right file names for LINUX 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 linux and
-         ;; initrd paths, to allow booting from a Btrfs subvolume.
-         #~(format port "menuentry ~s {
+          (device-mount-point (menu-entry-device-mount-point entry))
+          (multiboot-kernel (menu-entry-multiboot-kernel entry))
+          (chain-loader (menu-entry-chain-loader entry)))
+      (cond
+       (linux
+        (let ((arguments (menu-entry-linux-arguments entry))
+              (linux (normalize-file linux
+                                     device-mount-point
+                                     store-directory-prefix))
+              (initrd (normalize-file (menu-entry-initrd entry)
+                                      device-mount-point
+                                      store-directory-prefix)))
+          ;; Here DEVICE is the store and DEVICE-MOUNT-POINT is its mount point.
+          ;; Use the right file names for LINUX 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 linux and
+          ;; initrd paths, to allow booting from a Btrfs subvolume.
+          #~(format port "menuentry ~s {
   ~a
   linux ~a ~a
   initrd ~a
 }~%"
-                   #$label
-                   #$(grub-root-search device linux)
-                   #$linux (string-join (list #$@arguments))
-                   #$initrd))
-          (let ((kernel (menu-entry-multiboot-kernel entry))
-                (arguments (menu-entry-multiboot-arguments entry))
-                (modules (menu-entry-multiboot-modules entry))
-                (root-index 1))            ; XXX EFI will need root-index 2
-        #~(format port "
+                    #$label
+                    #$(grub-root-search device linux)
+                    #$linux (string-join (list #$@arguments))
+                    #$initrd)))
+       (multiboot-kernel
+        (let ((kernel (menu-entry-multiboot-kernel entry))
+              (arguments (menu-entry-multiboot-arguments entry))
+              (modules (menu-entry-multiboot-modules entry))
+              (root-index 1))            ; XXX EFI will need root-index 2
+          #~(format port "
 menuentry ~s {
   multiboot ~a root=device:hd0s~a~a~a
+}~%"
+                    #$label
+                    #$kernel
+                    #$root-index (string-join (list #$@arguments) " " 'prefix)
+                    (string-join (map string-join '#$modules)
+                                 "\n  module " 'prefix))))
+       (chain-loader
+        #~(format port "
+menuentry ~s {
+  ~a
+  chainloader ~a
 }~%"
                   #$label
-                  #$kernel
-                  #$root-index (string-join (list #$@arguments) " " 'prefix)
-                  (string-join (map string-join '#$modules)
-                               "\n  module " 'prefix))))))
+                  #$(grub-root-search device chain-loader)
+                  #$chain-loader)))))
 
   (define (crypto-devices)
     (define (crypto-device->cryptomount dev)
-- 
2.37.2





Information forwarded to guix-patches <at> gnu.org:
bug#57496; Package guix-patches. (Sun, 04 Sep 2022 17:30:02 GMT) Full text and rfc822 format available.

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

From: typ22 <at> foxmail.com
To: 57496 <at> debbugs.gnu.org
Cc: julien <at> lepiller.eu, tiantian <typ22 <at> foxmail.com>
Subject: [PATCH v4 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Mon,  5 Sep 2022 01:25:42 +0800
From: tiantian <typ22 <at> foxmail.com>

* gnu/bootloader.scm (report-menu-entry-error): New procedure.
(menu-entry->sexp): Add a call to `report-menu-entry-error'.

Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
---
v3:
The error message like so:

guix system: error: invalid menu-entry: #<<menu-entry> label: "test" device: #<file-system-label "guix-root"> device-mount-point: #f linux: #f linux-arguments: () initrd: #f multiboot-kernel: #f multiboot-arguments: () multiboot-modules: () chain-loader: #f>
hint: Please chose only one of:

  1. direct boot by specifying fields `linux', `linux-arguments' and `linux-modules',

  2. multiboot by specifying fields `multiboot-kernel', `multiboot-arguments' and `multiboot-modules',

  3. chain-loader by specifying field `chain-loader'.

The code of `report-menu-entry-error' is quoted from Julien Lepiller.
Thanks the help from Julien Lepiller.

v4:
The checks of the lists are cancelled and the check of initrd is moved
to the first patch.

 gnu/bootloader.scm | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index 9fe6b65212..da65b9d5d5 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -34,6 +34,8 @@ (define-module (gnu bootloader)
   #:use-module (guix diagnostics)
   #:use-module (guix i18n)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:export (menu-entry
             menu-entry?
@@ -110,6 +112,23 @@ (define-record-type* <menu-entry>
   (chain-loader     menu-entry-chain-loader
                     (default #f)))         ; string, path of efi file
 
+(define (report-menu-entry-error menu-entry)
+  (raise
+   (condition
+    (&message
+     (message
+      (format #f (G_ "invalid menu-entry: ~a") menu-entry)))
+    (&fix-hint
+     (hint
+      (G_ "Please chose only one of:
+@enumerate
+@item direct boot by specifying fields @code{linux},
+@code{linux-arguments} and @code{linux-modules},
+@item multiboot by specifying fields @code{multiboot-kernel},
+@code{multiboot-arguments} and @code{multiboot-modules},
+@item chain-loader by specifying field @code{chain-loader}.
+@end enumerate"))))))
+
 (define (menu-entry->sexp entry)
   "Return ENTRY serialized as an sexp."
   (define (device->sexp device)
@@ -146,7 +165,8 @@ (define (menu-entry->sexp entry)
                   (label ,label)
                   (device ,(device->sexp device))
                   (device-mount-point ,mount-point)
-                  (chain-loader ,chain-loader)))))
+                  (chain-loader ,chain-loader)))
+    (_ (report-menu-entry-error entry))))
 
 (define (sexp->menu-entry sexp)
   "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a <menu-entry>
-- 
2.37.2





Reply sent to Julien Lepiller <julien <at> lepiller.eu>:
You have taken responsibility. (Thu, 08 Sep 2022 20:34:02 GMT) Full text and rfc822 format available.

Notification sent to typ22 <at> foxmail.com:
bug acknowledged by developer. (Thu, 08 Sep 2022 20:34:02 GMT) Full text and rfc822 format available.

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

From: Julien Lepiller <julien <at> lepiller.eu>
To: typ22 <at> foxmail.com
Cc: 57496-done <at> debbugs.gnu.org
Subject: Re: [PATCH v4 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Thu, 8 Sep 2022 22:33:06 +0200
Pushed to master as 52d780ea2b0714d035a84e350b516ca2e2c19af1 to
32da9bbc91d365f514ae41528587905b21c41825, thanks!




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 07 Oct 2022 11:24:13 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 173 days ago.

Previous Next


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