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

[Danny Milosavljevic <dannym@scratchpost.org>]

[-- Attachment #1: Type: text/plain, Size: 6541 bytes --]

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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]