added grub bootloader for USB #2655

DEvil0000 · 2021-07-12T17:53:16Z

Type: New Feature / Enhancement
Impact: Normal
Reference to related issue (URL): partially OUTPUT=USB bug summary #2648
How was this pull request tested?

tested output usb with:

USB_BOOTLOADER=
USB_BOOTLOADER=grub
on apu2 board (coreboot/seabios) - no efi

Brief description of the changes in this pull request:
added option to write grub bootloader to usb (no efi)
it may not be perfect but a good starting point

usr/share/rear/output/USB/Linux-i386/300_create_extlinux.sh

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh

usr/share/rear/output/USB/Linux-i386/850_make_USB_bootable.sh

jsmeix · 2021-07-14T09:18:42Z

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh

+# in prep/USB/Linux-i386/350_check_usb_disk.sh
+
+[ "$RAW_USB_DEVICE" -a "$REAL_USB_DEVICE" ]
+BugIfError "RAW_USB_DEVICE and REAL_USB_DEVICE should be already set"


Better avoid the ...IfError functions.
They do not work fail-safe, see the comment in
usr/share/rear/lib/_input-output-functions.sh
about "Using the ...IfError functions can result unexpected behaviour in certain cases"
currently at
https://github.com/rear/rear/blob/master/usr/share/rear/lib/_input-output-functions.sh#L773

We try to replace the ...IfError functions by plain bash code
where we find them when we change code
but we keep them as is in old code that we do not touch.

I carried the check over from somewhere else. Will change it

I can imagine wherefrom you copied it ;-)
cf. #2644 (comment)

In particular the USB bootloader setup code is rather old code e.g. 50% of the code in output/USB/Linux-i386/300_create_extlinux.sh is 10 years old (from 2011 according to what 'git blame' shows) so large parts of the USB bootloader setup code is not in compliance with https://github.com/rear/rear/wiki/Coding-Style

I was not 100% sure how to rewrite that so please double check my commit for that

jsmeix · 2021-07-14T09:19:29Z

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh

+    grub_version=$($GRUB_INSTALL --version | awk '{print $NF}' | cut -c1-1)
+
+    case ${grub_version} in
+        0)


See "Paired parentheses" in
https://github.com/rear/rear/wiki/Coding-Style

jsmeix

See my comments at the code places

jsmeix · 2021-07-14T09:28:50Z

@DEvil0000
thank you for this very much missing enhancement!

I appreciate the new USB_BOOTLOADER config variable
because - at least for me - it was an endless confusion
what "install a bootloader" actually means in ReaR
because in ReaR there are two separated bootloader install actions:

Installing the bootloader of the ReaR recovery system during "rear mkrescue"
versus
(re)-installing the bootloader of the recreated system during "rear recover".

With clearly named config variables like USB_BOOTLOADER
it is now at least clear what bootloader will be used
for the ReaR recovery system when it is made with OUTPUT=USB

jsmeix · 2021-07-19T07:09:59Z

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh

@@ -0,0 +1,64 @@
+# only run for grub
+if [ -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ grub ]]; then
+    return 0


I do not undrestand the condition. I think

if [ -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ grub ]]

means

if $USB_BOOTLOADER is empty AND $USB_BOOTLOADER is not "grub"

but I assume output/USB/Linux-i386/300_create_grub.s should be skipped

if $USB_BOOTLOADER is empty OR $USB_BOOTLOADER is not "grub"

e.g.

# USB_BOOTLOADER='' ; if [ -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ grub ]] ; then echo no grub ; else echo grub ; fi no grub # USB_BOOTLOADER='grub' ; if [ -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ grub ]] ; then echo no grub ; else echo grub ; fi grub # USB_BOOTLOADER='foo' ; if [ -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ grub ]] ; then echo no grub ; else echo grub ; fi grub

I think the test should be simply

if $USB_BOOTLOADER is not "grub"

i.e.

# Skip when USB_BOOTLOADER is not 'grub': test "$USB_BOOTLOADER" = "grub" || return 0

the empty is in there for empty beeing previous behaviour as described in the conf file comment.
the -z is very common and readable bash to me for empty checks.
the not equal check van be written differently of course.

jsmeix · 2021-07-19T07:13:38Z

usr/share/rear/output/USB/Linux-i386/850_make_USB_bootable.sh

@@ -1,3 +1,8 @@
+# Only run for syslinux and extlinux
+if [ ! -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ syslinux|extlinux ]]; then


I do not see where USB_BOOTLOADER is set to syslinux or extlinux.
Similar as https://github.com/rear/rear/pull/2655/files#r672040064
I think the test should be simplified to

# Skip if USB_BOOTLOADER is 'grub' # see its counterpart in output/USB/Linux-i386/300_create_grub.sh test "$USB_BOOTLOADER" = "grub" && return

it does not need to be set since empty is previous behaviour but may be set by config

jsmeix

I think the tests are not right.

jsmeix · 2021-07-19T07:16:58Z

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh

@@ -41,7 +41,7 @@ if has_binary grub-install grub2-install; then
    # What version of grub are we using
    # substr() for awk did not work as expected for this reason cut was used
    # First charecter should be enough to identify grub version
-    grub_version=$($GRUB_INSTALL --version | awk '{print $NF}' | cut -c1-1)
+    grub_version=$(( $GRUB_INSTALL --version | awk '{print $NF}' | cut -c1-1 ))


The paired parenthesis are for the case cases
not for the command substitution.

jsmeix · 2021-07-19T07:25:10Z

@DEvil0000
if you like and if you agree
I could do my requested changes
directly in your pull request code here.

DEvil0000 · 2021-07-19T08:22:52Z

feel free to do changes.
Do you need write access to the branch at PR source?

jsmeix · 2021-07-19T08:34:54Z

I have write access - this is why I asked you for permit.
I have write access because the pull request sources
are already within the ReaR upstream GitHub area,
e.g. one of your commits has URL

https://github.com/rear/rear/pull/2655/commits/249a977119251d0ce766aca7bf84514c301acca2

so the pull request sources already belong to
github.com/rear/rear where I have write access.

jsmeix · 2021-07-19T11:21:50Z

Strange.
I can no longer change pull request code via the GitHub web interface.
I know I could do this in the past.

@rear/contributors
if there are no objections I would like to merge it as is tomorrow morning and
then I will further adapt and enhance things via a subsequent pull request from me.

Because USB_BOOTLOADER is a new config variable
we are free to implement how it should behave.

Currently I am thinking about something like:

USB_BOOTLOADER empty or unset (the default): Automatism what USB bootloader is used
USB_BOOTLOADER="extlinux": Enforce using syslinux/extlinux as USB bootloader
USB_BOOTLOADER="grub": Enforce using grub2 as USB bootloader
USB_BOOTLOADER="efi": Enforce EFI boot for USB

This is currently only some offhanded idea...

DEvil0000 · 2021-07-19T12:20:06Z

keep in mind that efi is not very specific - it may be used with different bootloaders and efi payloads and so on.

jsmeix · 2021-07-19T12:36:58Z

Yes, I have had a look at output/USB/Linux-i386/100_create_efiboot.sh
and because of what I saw there I intentionally proposed the unspecific

USB_BOOTLOADER="efi": Enforce EFI boot for USB

to basically run output/USB/Linux-i386/100_create_efiboot.sh as is.

If needed we may later further enhance things like

USB_BOOTLOADER="efi": Enforce EFI boot for USB (automatism what bootloader is used)
USB_BOOTLOADER="efi grub": Enforce EFI boot for USB with grub2 as bootloader
USB_BOOTLOADER="efi elilo": Enforce EFI boot for USB with elilo as bootloader

jsmeix · 2021-07-19T13:01:52Z

I think EFI boot setup and BIOS boot setup
would normally exclude each other
but I fail to see how
output/USB/Linux-i386/100_create_efiboot.sh
and
output/USB/Linux-i386/300_create_extlinux.sh
exclude each other.

What I can see is that output/USB/Linux-i386/100_create_efiboot.sh
is only run when USING_UEFI_BOOTLOADER is true
but I fail to see how output/USB/Linux-i386/300_create_extlinux.sh
is not run when USING_UEFI_BOOTLOADER is true.

So - as far as I can currently see from plain looking at the code - it seems
output/USB/Linux-i386/300_create_extlinux.sh
is run in any case - in particular also on systems with UEFI firmware.

I wonder if that makes sense?

DEvil0000 · 2021-07-20T08:26:26Z

I did not test or change that configuration combination / path in software.

There may be good reasons for this. In case of elilo.efi it does not run the grub config part in 100_create_efiboot.sh what makes me believe this case needs some syslinux/extlinux config then.
Also there is still the option of creating a boot device which can boot from legacy bios and efi method but I think thats not what was intended here. I am however not familiar with this.

jsmeix · 2021-07-20T14:09:17Z

@DEvil0000
in your output/USB/Linux-i386/300_create_grub.sh there is

USB_REAR_DIR="$BUILD_DIR/outputfs/$USB_PREFIX"
if [ ! -d "$USB_REAR_DIR" ]; then
    mkdir -p $v "$USB_REAR_DIR" >/dev/null || Error "Could not create USB ReaR dir [$USB_REAR_DIR] !"
fi

but it seems USB_REAR_DIR is nowhere used
so I would like to know why that directory is created there
i.e. what the reason behind is why that directory is created?

USB_REAR_DIR also appears in output/USB/Linux-i386/300_create_extlinux.sh
but there it is used to write syslinux things there.

Is creating USB_REAR_DIR in output/USB/Linux-i386/300_create_grub.sh
perhaps only an obsolete leftover from some copy&paste?

DEvil0000 · 2021-07-21T11:10:38Z

Maybe the directory USB_REAR_DIR should be created elsewhere but it is used to store the kernel and initrd later as well as parts of syslinux config if syslinux is used. For one of the following scripts it needed to exist.

jsmeix · 2021-07-21T12:44:58Z

@DEvil0000
thank you for the info!

What a mess that old USB related code is!
The global looking variable USB_REAR_DIR is not used globally
but the $BUILD_DIR/outputfs/$USB_PREFIX directory is needed
so for now I at least explained things via
b20858a

added grub bootloader for USB

249a977

jsmeix reviewed Jul 14, 2021

View reviewed changes

usr/share/rear/output/USB/Linux-i386/300_create_extlinux.sh Outdated Show resolved Hide resolved

jsmeix reviewed Jul 14, 2021

View reviewed changes

usr/share/rear/output/USB/Linux-i386/300_create_grub.sh Outdated Show resolved Hide resolved

jsmeix reviewed Jul 14, 2021

View reviewed changes

usr/share/rear/output/USB/Linux-i386/850_make_USB_bootable.sh Show resolved Hide resolved

jsmeix reviewed Jul 14, 2021

View reviewed changes

jsmeix requested changes Jul 14, 2021

View reviewed changes

jsmeix requested a review from a team July 14, 2021 09:29

jsmeix self-assigned this Jul 14, 2021

jsmeix added the enhancement Adaptions and new features label Jul 14, 2021

jsmeix added this to the ReaR v2.7 milestone Jul 14, 2021

DEvil0000 added 3 commits July 16, 2021 18:46

remvoed shopt for nocasematch option

06ec2e6

fixed paired parenthesis

0f87532

replacing some ...IfError functions

8ab4cc1

jsmeix reviewed Jul 19, 2021

View reviewed changes

jsmeix requested changes Jul 19, 2021

View reviewed changes

jsmeix reviewed Jul 19, 2021

View reviewed changes

jsmeix merged commit 301153f into rear:master Jul 20, 2021

jsmeix added the fixed / solved / done label Jul 20, 2021

jsmeix mentioned this pull request Jul 20, 2021

Simplifications for USB_BOOTLOADER="grub" #2659

Merged

jsmeix added the waiting for info label Jul 20, 2021

jsmeix removed the waiting for info label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added grub bootloader for USB #2655

added grub bootloader for USB #2655

DEvil0000 commented Jul 12, 2021

jsmeix Jul 14, 2021

DEvil0000 Jul 14, 2021

jsmeix Jul 14, 2021

DEvil0000 Jul 16, 2021

jsmeix Jul 14, 2021

jsmeix left a comment

jsmeix commented Jul 14, 2021

jsmeix Jul 19, 2021

DEvil0000 Jul 19, 2021

jsmeix Jul 19, 2021

DEvil0000 Jul 19, 2021

jsmeix left a comment

jsmeix Jul 19, 2021

jsmeix commented Jul 19, 2021

DEvil0000 commented Jul 19, 2021

jsmeix commented Jul 19, 2021

jsmeix commented Jul 19, 2021 •

edited

DEvil0000 commented Jul 19, 2021

jsmeix commented Jul 19, 2021 •

edited

jsmeix commented Jul 19, 2021

DEvil0000 commented Jul 20, 2021

jsmeix commented Jul 20, 2021

DEvil0000 commented Jul 21, 2021

jsmeix commented Jul 21, 2021

		@@ -1,3 +1,8 @@
		# Only run for syslinux and extlinux
		if [ ! -z $USB_BOOTLOADER ] && [[ ! $USB_BOOTLOADER =~ syslinux\|extlinux ]]; then

added grub bootloader for USB #2655

added grub bootloader for USB #2655

Conversation

DEvil0000 commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmeix left a comment

Choose a reason for hiding this comment

jsmeix commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmeix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsmeix commented Jul 19, 2021

DEvil0000 commented Jul 19, 2021

jsmeix commented Jul 19, 2021

jsmeix commented Jul 19, 2021 • edited

DEvil0000 commented Jul 19, 2021

jsmeix commented Jul 19, 2021 • edited

jsmeix commented Jul 19, 2021

DEvil0000 commented Jul 20, 2021

jsmeix commented Jul 20, 2021

DEvil0000 commented Jul 21, 2021

jsmeix commented Jul 21, 2021

jsmeix commented Jul 19, 2021 •

edited

jsmeix commented Jul 19, 2021 •

edited