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

[Danny Milosavljevic]

Hi Stefan,

On Tue, 8 Sep 2020 00:59:38 +0200
Stefan <stefan-guix@vodafonemail.de> wrote:

> In the end this is a grub-efi for booting over network. 

>Would grub-efi-netboot be an even better name? It will not work with BIOS 
>machines.

Oh, then definitely let's use that name.

> I only need the first list element here. I will use (first …).

Okay.

(I leave it to the others to comment on here if they have a problem with it--I
see no downside in this case)

> >> +         (efi-bootloader-link (string-append "/boot"  
> >   
> >> +                                             (match arch
> >> +                                               ("i686" "ia32")
> >> +                                               ("x86_64" "x64")
> >> +                                               ("arm" "arm")
> >> +                                               ("armhf" "arm")
> >> +                                               ("aarch64" "aa64")
> >> +                                               ("riscv" "riscv32")
> >> +                                               ("riscv64" "riscv64"))
> >> +                                             ".efi"))  
> > 
> > Also, I have a slight preference for greppable file names even when it's a
> > little more redundant, so more like that:
> > 
> > (match system-parts
> > (("i686" _ ...) "ia32.efi")
> > (("x86_64" _ ...) "x64.efi")
> > (("arm" _ ...) "arm.efi")
> > (("armhf" _ ...) "arm.efi")
> > (("aarch64" _ ...) "aa64.efi")
> > (("riscv" _ ...) "riscv32.efi")
> > (("riscv64" _ ...) "riscv64.efi"))  
> 
> I’m not familiar with the match syntax yet. For me using the first element as 
> arch seems simpler.

Match just does pattern matching.  The pattern here is for example ("i686" _ 
...).
That means it will match anything that is a list that is starting with "i686".
It will put the remainder (...) into the variable "_" (which is customary to
use as "don't care" variable).

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.

You have used "match" before--but only on parts of the list.  Why not use it
on the whole list?  It makes little sense to do manual destructuring and then
use match--when match would have done the destructuring bind anyway.

> > Likewise:
> > 
> >         (efi-bootloader (match system-parts
> >                          (("i686" _ ...) "i386-efi/core.efi")
> >                          (("x86_64" _ ...) "x86_64-efi/core.efi")
> >                          (("arm" _ ...) "arm-efi/core.efi")
> >                          (("armhf" _ ...) "arm-efi/core.efi")
> >                          (("aarch64" _ ...) "arm64-efi/core.efi")
> >                          (("riscv" _ ...) "riscv32-efi/core.efi")
> >                          (("riscv64" _ ...) "riscv64-efi/core.efi"))))  
> 
> I’d prefer to keep the still grepable “/core.efi” separate.

Sure.

> And yes, when using ‘guix system init /etc/config.scm /mnt/here’, then 
> MOUNT-POINT and TARGET are concatenated. But this is nothing specific to the 
> new installer, this is the usual behaviour of Guix and the reason for the two 
> parameters TARGET and MOUNT-POINT to any bootloader installer. I don’t think 
> stating this inside the new doc-string is the right place.

Ah, so that's what it means.

Well, it should be stated *somewhere* at least.  It probably is and I just
didn't see it.

> Yes, correct. I’ll rework this with the (symlink-relative) function you 
> mentioned.

Thanks!

> > So TARGET is relative to MOUNT-POINT ?
> > And MOUNT-POINT is assumed to have a slash at the end ?  
> 
> MOUNT-POINT is either ‘/’ or depends on the argument to ‘guix system init’. 
> On the other side TARGET has to be an absolute path, so it should be safe. At 
> least (install-grub-efi) makes the same mistake. What do you think?

If grub-efi does it then it seems to be fine to do it--at least we didn't get
bug reports caused by it.  Let's just keep using it for the time being.

> >> +               (store-name (car (delete "" (string-split bootloader 
> >> #\/))))  
> > 
> > Maybe use match.  
> 
> I’ll use (first …).
> 
> > Also isn't there an official way to find out how the store is called ?
> > (%store-prefix) ?  
> 
> I only need the first path element to the store, which is usually /gnu. The 
> %store-prefix contains /gnu/store then. So it makes no difference.

I have no strong opinion either way, except please add a comment that you
are extracting part of the store prefix (or whatever) from the in-store
name of the bootloader store item.  It seems weird to me to do that--but
then again I don't get why Guix has two directories (/gnu and /gnu/store)
to the store anyway.

Fine, I guess.

I'm not sure whether it would be technically possible to have a custom
store directory like "/foo" without "/gnu" as the store.  That would be
a problem--and I'm sure someone somewhere does that--otherwise, why have
%store-prefix as a variable otherwise?

> >> +               (store (string-append up1 store-name))  
> > 
> > (string-append escape-target store-name)
> >   
> >> +               (store-link (string-append net-dir store-name))  
> > 
> > *mumbles to self* (string-append MOUNT-POINT TARGET) is net-dir.
> > So it tries to get to (string-append MOUNT-POINT "/gnu").  
> 
> The trouble is that GRUB shall load a file like /gnu/store/…-linux…/Image via 
> TFTP, but the TFTP root is actually Guix’ final /boot folder.
> 
> In the end this creates a relative symlink as ../gnu pointing from 
> /mnt/here/boot/gnu to /mnt/here/gnu.
> 
> And GRUB’s “working directory” to search for its modules and the grub.cfg is 
> defined by its ‘prefix’ variable, which is set through the SUBDIR argument, 
> which defaults to Guix’ final /boot/efi/Guix.
> 
> This requires a relative symlink as ../../../boot/grub/grub.cfg pointing from 
> /mnt/here/boot/efi/Guix/grub.cfg to /mnt/here/boot/grub/grub.cfg.
> 
> And be aware that TARGET may be /boot, but could be something else like 
> /tftp-root. Then the symlink would point from 
> /mnt/here/tftp-root/efi/Guix/grub.cfg to /mnt/here/boot/grub/grub.cfg, as the 
> later is kind of hard-coded.

Please add that to comments in the source code.  Otherwise, it would be
very probable to be broken by further maintenance.

pgpFS_sMkg93N.pgp
Description: OpenPGP digital signature