GNU bug report logs - #57496
[PATCH 0/2] Add support for chain-loader

Previous Next

Full log

Message #59 received at 57496 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: typ22 <at> foxmail.com
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Sun, 4 Sep 2022 17:37:55 +0200

Hi tiantian,

I think the first two patches are good now, so let me focus on this one
:)

Le Sun,  4 Sep 2022 22:04:31 +0800,
typ22 <at> foxmail.com a écrit :

> From: tiantian <typ22 <at> foxmail.com>
> 
> +  (define (set-field? field)
> +    "If the field is the default value, return #f."
> +    (and field                  ; not default value #f
> +         (not (null? field))))  ; not default value '()

I don't think this set-field? is necessary. In the following, I don't
think you need the null? case at all because I think all the lists may
be empty without triggering an error. You don't necessarily want to
load modules or have arguments on the linux command line.

In any case, it should be called field-set? instead :)

>    (match entry
> +    ;; Modify the pattern to achieve more strict matching and prevent
> +    ;; wrong matching, which ensures the output of error information
> +    ;; when menu-entry is wrong.
> +
> +    ;; The linux-arguments is optional and the test code for
> boot-parameters
> +    ;; does not set it, so don't check it here.
>      (($ <menu-entry> label device mount-point
> -                     (? identity linux) linux-arguments initrd
> +                     (? set-field? linux)
> +                     linux-arguments
> +                     (? set-field? initrd)

The could still be identity

>                       #f () () #f)
>       `(menu-entry (version 0)
>                    (label ,label)
> @@ -131,8 +162,10 @@ (define (menu-entry->sexp entry)
>                    (linux-arguments ,linux-arguments)
>                    (initrd ,initrd)))
>      (($ <menu-entry> label device mount-point #f () #f
> -                     (? identity multiboot-kernel)
> multiboot-arguments
> -                     multiboot-modules #f)
> +                     (? set-field? multiboot-kernel)
> +                     (? set-field? multiboot-arguments)
> +                     (? set-field? multiboot-modules)

Some users might want to not use any modules or arguments I think, so
these fields should not be mandatory. For multiboot-kernel, identity is
enough.

> +                     #f)
>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
> @@ -141,12 +174,13 @@ (define (menu-entry->sexp entry)
>                    (multiboot-arguments ,multiboot-arguments)
>                    (multiboot-modules ,multiboot-modules)))
>      (($ <menu-entry> label device mount-point #f () #f #f () ()
> -                     (? identity chain-loader))
> +                     (? set-field? chain-loader))

Same here, identity is fine.

>       `(menu-entry (version 0)
>                    (label ,label)
>                    (device ,(device->sexp device))
>                    (device-mount-point ,mount-point)
> -                  (chain-loader ,chain-loader)))))
> +                  (chain-loader ,chain-loader)))
> +    (else (report-menu-entry-error entry))))

Since this is a match, it is more common to use:

(_ (report-menu-entry-error entry))

Also, it feels weird to patch the code you modified in a previous patch
of the same series. If you're not happy with the code you wrote in a
previous patch, you need to change it in the previous patch, not in a
new one :)

>  
>  (define (sexp->menu-entry sexp)
>    "Turn SEXP, an sexp as returned by 'menu-entry->sexp', into a
> <menu-entry>

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.