#2609 PR `merged`: Better description and error checking for GRUB_RESCUE with UEFI plus some alignment with create_grub2_cfg function¶

Labels: enhancement, documentation, fixed / solved / done

jsmeix opened issue at 2021-05-04 10:19:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL):
https://github.com/rear/rear/issues/2545
How was this pull request tested?
Not at all tested by me - currently I don't have a UEFI test system
Brief description of the changes in this pull request:
Improved user messages during GRUB_RESCUE setup in particular for the UEFI case
plus some error checking in the UEFI case and some minor code simplifications.

jsmeix commented at 2021-05-04 10:33:¶

@pcahyna @gozora
currently we have in output/default/940_grub2_rescue.sh (excerpt)

    # Create configuration file for "Relax-and-Recover" UEFI boot entry.
    # This file will not interact with existing Grub2 configuration in any way.
    (   echo "set btrfs_relative_path=y"
        echo "insmod efi_gop"
        echo "insmod efi_uga"
        echo "insmod video_bochs"
        echo "insmod video_cirrus"
        echo "insmod all_video"
...
    ) > $grub_config_dir/rear.cfg

We cleaned up that kind of explicit insmod GRUB_module specifications
some time ago because those had caused various issues, e.g. see
https://github.com/rear/rear/pull/2390

@pcahyna @gozora
could you please have a look here if same kind of cleanup
is perhaps also needed in output/default/940_grub2_rescue.sh
or correct me if I am wrong.
Thank you in advance!

pcahyna commented at 2021-05-04 11:25:¶

@jsmeix storage modules are detected automatically, but I had to re-add video modules in 361268355f493aeb0c22069c3e143bdba5be7203, otherwise graphics did not work. We then cleaned up those modules in 2470edf534bc836a3f13d8c48e3007a24bb63bf4 and 0cc1b6a7450133f952b8c187e85d61397a45dbe1 because we determined that all_video is enough: discussion in #2388.
Unfortunately, the change did not propagate from create_grub2_cfg() in bootloader-functions.sh to here, because 940_grub2_rescue.sh has its own version of GRUB2 config code (see the XXX in 361268355f493aeb0c22069c3e143bdba5be7203)

jsmeix commented at 2021-05-04 11:54:¶

@pcahyna
thank you for finding the matching git commits!

I like to summarize:

On Dec 15, 2019
the GRUB2 modules efi_gop efi_uga video_bochs video_cirrus all_video
and set gfxpayload=keep were added to 940_grub2_rescue.sh via
https://github.com/rear/rear/commit/361268355f493aeb0c22069c3e143bdba5be7203

On May 7, 2020
the GRUB2 modules video_bochs video_cirrus were removed from create_grub2_cfg via
https://github.com/rear/rear/commit/0cc1b6a7450133f952b8c187e85d61397a45dbe1

On May 13, 2020
the GRUB2 modules efi_gop efi_uga were removed from create_grub2_cfg via
https://github.com/rear/rear/commit/2470edf534bc836a3f13d8c48e3007a24bb63bf4

So now there is in create_grub2_cfg only

insmod all_video

set gfxpayload=keep
insmod gzio
insmod part_gpt
insmod ext2

while in 940_grub2_rescue.sh we have currently still

insmod efi_gop
insmod efi_uga
insmod video_bochs
insmod video_cirrus
insmod all_video
set gfxpayload=keep

According to
https://github.com/rear/rear/commit/361268355f493aeb0c22069c3e143bdba5be7203

XXX the Grub2 config file generation for GRUB2_RESCUE should be unified with
config file generation for ISO (create_grub2_cfg).

things in 940_grub2_rescue.sh should match what we meanwhile have in create_grub2_cfg
so I should change 940_grub2_rescue.sh so that the code there becomes

    # Create configuration file for "Relax-and-Recover" UEFI boot entry.
    # This file will not interact with existing Grub2 configuration in any way.
    (   echo "set btrfs_relative_path=y"
        echo "insmod all_video"
        echo ""
        echo "set gfxpayload=keep"

Is that right?
If yes I would change 940_grub2_rescue.sh accordingly.

What about

insmod gzio
insmod part_gpt
insmod ext2

that we have in create_grub2_cfg but not in 940_grub2_rescue.sh ?

pcahyna commented at 2021-05-04 12:26:¶

things in 940_grub2_rescue.sh should match what we meanwhile have in create_grub2_cfg
so I should change 940_grub2_rescue.sh so that the code there becomes
    # Create configuration file for "Relax-and-Recover" UEFI boot entry.
    # This file will not interact with existing Grub2 configuration in any way.
    (   echo "set btrfs_relative_path=y"
        echo "insmod all_video"
        echo ""
        echo "set gfxpayload=keep"
Is that right?
If yes I would change 940_grub2_rescue.sh accordingly.

Would it be feasible to just use create_grub2_cfg() in 940_grub2_rescue.sh instead of carrying a copy of the code?

What about
insmod gzio
insmod part_gpt
insmod ext2
that we have in create_grub2_cfg but not in 940_grub2_rescue.sh ?

I don't know. This seemed a bit arbitrary (why ext2? What if we are using some other filesystem?) but I avoided touching that part when changing the GRUB2 stuff, because it was not causing any immediate problems. Looking at log, it was added in 33305d0fa without any explanation and it appeared de novo (the tree at that point does no have any other occurrence of insmod ext2).

jsmeix commented at 2021-05-04 12:33:¶

I am neither a sufficient UEFI expert nor a sufficient GRUB2 expert
to implement using create_grub2_cfg() in 940_grub2_rescue.sh

Additionally this would be a too major change that exceeds too much
the intent of this pull request.

Of course we could need some major cleanup of the whole grown
various pieces of different bootloader related code parts in ReaR,
e.g. cf. https://github.com/rear/rear/issues/764

jsmeix commented at 2021-05-04 12:45:¶

Via
https://github.com/rear/rear/pull/2609/commits/3eb8f0589d6d2d6b49bf589137517f36b675f9dc
the GRUB2 configuration file that is created by output/default/940_grub2_rescue.sh
does no longer "insmod" the GRUB2 modules efi_gop efi_uga video_bochs video_cirrus
to create this GRUB2 configuration file like the create_grub2_cfg function does it
cf. https://github.com/rear/rear/pull/2609#issuecomment-831883795

When this is wrong I can revert it back to what it was before or change it
to whatever I am told what "the right list of GRUB2 modules" should be here.

jsmeix commented at 2021-05-07 09:49:¶

@gozora @rear/contributors
could you have a look here and ideally do a review provided you have time for it?
If not I would like to merge it as is "bona fide" on Monday afternoon.

jsmeix commented at 2021-05-10 13:21:¶

@pcahyna
thank you for your review.
Currently I cannot do the needed complete cleanup regarding
our various different kind of UEFI boot setup code places
so I only added a TODO comment to have it at least described.

If there are no objections I would like to merge it as is tomorrow afternoon.

pcahyna commented at 2021-05-10 14:59:¶

@jsmeix no further comment on the changes from me, a TODO is enough for the scope of the PR, but I think you should adjust the PR title and the merge commit message, because in addition to improving user messages you did some functional changes (removing the GRUB video modules from generated config).

jsmeix commented at 2021-05-11 10:13:¶

@pcahyna
thank you for all your help here to move one step forward in the UEFI area.

[Export of Github issue for rear/rear.]

#2609 PR merged: Better description and error checking for GRUB_RESCUE with UEFI plus some alignment with create_grub2_cfg function¶