GNU bug report logs - #41011
[PATCH] gnu: grub: Support for network boot via tftp/nfs.

Previous Next

Full log

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

From: Stefan <stefan-guix <at> vodafonemail.de>
To: Efraim Flashner <efraim <at> flashner.co.il>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 41011 <at> debbugs.gnu.org
Subject: Re: [bug#41011] [PATCH] gnu: grub: Support for network boot via TFTP.
Date: Tue, 15 Sep 2020 22:28:53 +0200

Hi Efraim!

>> +         (boot-efi-link (match system
>> +                          (("i686" _ ...) "/bootia32.efi")
>> +                          (("x86_64" _ ...) "/bootx64.efi")
>> +                          (("arm" _ ...) "/bootarm.efi")
>> +                          (("armhf" _ ...) "/bootarm.efi")
>> +                          (("aarch64" _ ...) "/bootaa64.efi")
>> +                          (("riscv" _ ...) "/bootriscv32.efi")
>> +                          (("riscv64" _ ...) "/bootriscv64.efi")))
> 
> Don't forget the fall-through case, even if it's just
> ((_ ...) "/bootTODO.efi")

There was a contradicting remark by Danny:

> The major advantage of using "match" is its failure mode.  If the thing matched
> on is not a list (for some unfathomable reason) or if the first element is not
> matched on (!) then you get an exception--which is much better than doing weird
> unknown stuff.

Actually I would prefer an error here as well. Imagine a successful ‘guix system init’ silently creating a bootTODO.efi. Booting that system will certainly fail and someone will have a hard time to figure out that the generated bootTODO.efi is the reason.

>> +         (efi-bootloader (string-append (match system
>> +                                          (("i686" _ ...) "i386-efi")
>> +                                          (("x86_64" _ ...) "x86_64-efi")
>> +                                          (("arm" _ ...) "arm-efi")
>> +                                          (("armhf" _ ...) "arm-efi")
>> +                                          (("aarch64" _ ...) "arm64-efi")
>> +                                          (("riscv" _ ...) "riscv32-efi")
>> +                                          (("riscv64" _ ...) "riscv64-efi"))
>> +                                        "/core.efi")))
> 
> With the fall-through case here, can this be changed to
> (("i686" _ ...) "i386-efi")
> (("aarch64" _ ...) "arm64-efi")
> (("riscv" _ ...) "riscv32-efi")
> ((_ ...) (string-append (first
>                         (string-split
>                           (nix-system->gnu-triplet
>                             (or (%current-system)
>                                 (%current-target-system)))
>                           #\_))
>                        "-efi"))

There was a contradicting remark by Danny, which applies to the first part as well:

> Also, I have a slight preference for greppable file names even when it's a
> little more redundant

I understand your point in generating the arch part. I also understand the usage of (nix-system->gnu-triplet) to convert armhf to arm. I also think the risk is low that the first part raises no error and this part is doing something wrong without raising an error, too, leading to a not booting system. So having a default here seems fine to me.

However, Danny’s argument is convincing, too. And keeping this block similar to the first block eases the understanding, at least mine. My first thought seeing your suggestion was: “Why does this block need to be different from the other and what is this code doing differently?”

What do you think?


Bye

Stefan

Previous Next

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