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

Previous Next

Full log

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

From: Efraim Flashner <efraim <at> flashner.co.il>
To: Stefan <stefan-guix <at> vodafonemail.de>
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: Wed, 16 Sep 2020 10:51:17 +0300

[Message part 1 (text/plain, inline)]

On Tue, Sep 15, 2020 at 10:28:53PM +0200, Stefan wrote:
> 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.
> 

My concern is more for architectures which aren't on the list having
unfortunate errors while doing something unrelated. Another option then
I suppose would be
((_ ...) #f)
It should still fail if you try to use it but there's still a code path
for, say, ppc64el. I like the #f idea better than bootTODO.efi.

> >> +         (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"))

I re-noticed that system is passed above so my code block could be a bit
more contained

((_ ...) (string-append (first
                          (string-split
                            (nix-system->gnu-triplet 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?
> 

The benefit is that there's less chance of typoing a mistake in the
code, either when writing it or when editing it later. There's also the
benefit again of not dismissing architectures which are currently not
listed in the list.

For something greppable, I don't really have a counter argument. Perhaps
a hand-wavey "code correctness" of reusing macros.

For unsupported architectures, ((_ ...) #f) again would work to make
sure there is at least a code path which would definitely fail if
someone tried to use it. That's my primary concern.

> 
> Bye
> 
> Stefan
> 

Thanks

-- 
Efraim Flashner   <efraim <at> flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[signature.asc (application/pgp-signature, inline)]

Previous Next

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