[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