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

From: Julien Lepiller <[email-masked]>
Date: Sat, 3 Sep 2022 22:08:35 +0200

Hi tiantian,

Le Fri, 02 Sep 2022 09:04:15 +0800,
tiantian <[email-masked]> 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 <[email-masked]>

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