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

Previous Next

Full log

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

From: tiantian <typ22 <at> foxmail.com>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 57496 <at> debbugs.gnu.org
Subject: Re: [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of
 menu-entry.
Date: Mon, 05 Sep 2022 00:15:12 +0800

Julien Lepiller <julien <at> lepiller.eu> writes:

> 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 :)
>

My English is not good. To be honest, I tried set-value?, value-set?,
default-value?, not-default-value?, field-set? and set-field?. Finally,
I select the 'set-field?'. But it seems that I didn't choose well.

But it doesn't matter. The procedure is no longer needed.

>>    (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.
>

I don't know multiboot very well, so I don't know if multiboot-arguments
and multiboot-modules are necessary. Then I check them. It turns out that
they are not necessary. I will not check them.

I will change to use identify. Honestly, It's much easier for me to
use only `identify'. :)

>>       `(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))
>

Thank you for reminding me. I will correct it.

> 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 :)
>

As I understood earlier, these changes about matching are related to the
error reporting information, so I put these modifications in this
submission. My knowledge of contribution is still too little.

I will pay attention to it later. Thank you for reminding me.

Thanks,
tiantian

Previous Next

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