[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