[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[typ22]

From: tiantian <typ22@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)))))
 
 \f
 ;;;
-- 
2.37.2

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[Julien Lepiller]

[-- Attachment #1: Type: text/plain, Size: 6099 bytes --]

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@foxmail.com a écrit :
>From: tiantian <typ22@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)))))
> 
> \f>
> ;;;
>-- 
>2.37.2
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 6641 bytes --]

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[Julien Lepiller]

Le 31 août 2022 09:18:44 GMT+02:00, Julien Lepiller <julien@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@foxmail.com a écrit :
>>From: tiantian <typ22@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)))))
>> 
>> \f>>
>> ;;;
>>-- 
>>2.37.2
>>
>>
>>
>>

[bug#57496] [PATCH 2/2] gnu: bootloader: grub: Add support for chain-loader.

[typ22]

From: tiantian <typ22@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

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[Julien Lepiller]

Le Thu, 1 Sep 2022 01:55:34 +0800,
tiantian <typ22@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@foxmail.com>
> To: Julien Lepiller <julien@lepiller.eu>, 57496@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@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@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@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
> 
> 

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[tiantian]

Hi,

Julien Lepiller <julien@lepiller.eu> writes:

> Le Thu, 1 Sep 2022 01:55:34 +0800,
> tiantian <typ22@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

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[tiantian]

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

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[Julien Lepiller]

Hi tiantian,

Le Fri, 02 Sep 2022 09:04:15 +0800,
tiantian <typ22@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@lepiller.eu>

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

[bug#57496] [PATCH 1/2] gnu: bootloader: Extend `<menu-entry>' for chain-loader.

[tiantian]

Hi Julien,

Julien Lepiller <julien@lepiller.eu> writes:

> Hi tiantian,
>
> Le Fri, 02 Sep 2022 09:04:15 +0800,
> tiantian <typ22@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@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@lepiller.eu>" to commit messages of my first and third patch.

Thanks,
tiantian