[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
> 
>