#1477 PR `closed`: Update 310_include_uefi_tools.sh¶

Labels: won't fix / can't fix / obsolete

jmazanek opened issue at 2017-09-08 11:14:¶

This change should resolve issue of rear terminating if /boot/efi directory is not mounted

jmazanek commented at 2017-09-08 11:17:¶

jsmeix commented at 2017-09-08 11:47:¶

@gdha
I added you as reviewer because
"git log -p --follow usr/share/rear/prep/default/310_include_uefi_tools.sh"
lists you as the main author.

@gozora
I added you as reviewer because it is about "UEFI".

gozora commented at 2017-09-08 11:51:¶

I'd say that whole condition

if [[ ! -d /boot/[eE][fF][iI] ]]; then
    if is_true $USING_UEFI_BOOTLOADER; then
        Error "USING_UEFI_BOOTLOADER = 1 but there is no directory at /boot/efi or /boot/EFI" # abort
    fi
    return # skip
 fi

is somehow "off topic".

Is there benefit from such check?

V.

gozora commented at 2017-09-08 12:03:¶

Hmm, OK maybe not fully "off topic", but maybe we could simplify it bit.

Something like

if ! is_true $USING_UEFI_BOOTLOADER; then
    return 
fi

V.

jsmeix commented at 2017-09-08 12:13:¶

@gozora cf.
https://github.com/rear/rear/commit/9bb07357ac7892ad7d82c5438e76e57a44d21113
and for more check
"git log -p --follow usr/share/rear/prep/default/310_include_uefi_tools.sh"

pcahyna commented at 2017-09-08 17:44:¶

I can reproduce the problem with rear-2.00, but not with the git master branch (revision 082807944a186a916b9f03235b8666568c349bc6). I suspect this fix does not apply to master anymore.

OliverO2 commented at 2017-09-08 20:20:¶

I have tested the current master branch on Ubuntu 16.04.3 LTS. On my UEFI boot system, rear mkrescue failed to include efibootmgr and thus rear recover failed to create a booting system. Seems like two effects contributed to the problem:

1. The test in line 7 of `310_include_uefi_tools.sh` returns the wrong result if `/boot/[eE][fF][iI]` exists:¶

Quoting the bash manual page:

Word splitting and pathname expansion are not performed on the words between the [[ and ]];

Changing the test to [ ! -d /boot/[eE][fF][iI] ] (single brackets) makes it work.

Script to demonstrate:

#!/usr/bin/env bash

ls -ld /boot/[eE][fF][iI]

if [[ ! -d /boot/[eE][fF][iI] ]]; then
    echo "/boot/[eE][fF][iI] not found (original test)"
else
    echo "/boot/[eE][fF][iI] found (original test)"
fi

if [ ! -d /boot/[eE][fF][iI] ]; then
    echo "/boot/[eE][fF][iI] not found (modified test)"
else
    echo "/boot/[eE][fF][iI] found (modified test)"
fi

Script output:

drwxr-xr-x 3 root root 512 Jan  1  1970 /boot/efi
/boot/[eE][fF][iI] not found (original test)
/boot/[eE][fF][iI] found (modified test)

2. `USING_UEFI_BOOTLOADER` is used before initalization¶

310_include_uefi_tools.sh requires a correctly set USING_UEFI_BOOTLOADER variable, but this is only set afterwards in 320_include_uefi_env.sh. Renaming 310_include_uefi_tools.sh to 330_include_uefi_tools.sh fixed that problem for me.

pcahyna commented at 2017-09-08 20:35:¶

@OliverO2

On my UEFI boot system, rear mkrescue failed to include efibootmgr and thus rear recover failed to create a booting system.

Thanks for the testing with EFI. When I wrote

I can reproduce the problem with rear-2.00, but not with the git master branch

I was speaking about the problem of rear not working on systems without EFI, and no statement about systems with EFI was intended.

Word splitting and pathname expansion are not performed on the words between the [[ and ]];

Indeed, see my recent comment https://github.com/jsmeix/rear/commit/9bb07357ac7892ad7d82c5438e76e57a44d21113#commitcomment-24177948 on 9bb07357ac7892ad7d82c5438e76e57a44d21113 (which introduced this problem).

USING_UEFI_BOOTLOADER is used before initalization

I also wondered why the variable is being set in 320_xxx and being used in 310_xxx .

jsmeix commented at 2017-09-11 09:26:¶

With
https://github.com/rear/rear/commit/6e05b6f0e51fdfdf02c67edddb117b41a364c6f4
I use simpler code in 310_include_uefi_tools.sh to fix
https://github.com/jsmeix/rear/commit/9bb07357ac7892ad7d82c5438e76e57a44d21113#commitcomment-24177948
and I did some general cleanup there.
Currently I don't have an UEFI test system running so that
I could not test it myself.
@pcahyna @OliverO2 please test if the current
GitHub master code works for you.

@gozora
please have a look if 310_include_uefi_tools.sh looks better now
(but I do not intend to do a complete overhaul of that script).

OliverO2 commented at 2017-09-11 10:43:¶

@jsmeix
I'll test later today. Looks like it will probably work but the problem with $USING_UEFI_BOOTLOADER being used before initialization in 320_include_uefi_env.sh remains. I'd suggest

renaming 310_include_uefi_tools.sh to 330_include_uefi_tools.sh and
relying on the UEFI presence checks already done in 320_include_uefi_env.sh replacing the two if statements at the top of in 330_include_uefi_tools.sh with this test:

is_true $USING_UEFI_BOOTLOADER || return

jsmeix commented at 2017-09-11 11:47:¶

@OliverO2
my immediate
https://github.com/rear/rear/commit/6e05b6f0e51fdfdf02c67edddb117b41a364c6f4
was only meant to fix your particular reported
https://github.com/jsmeix/rear/commit/9bb07357ac7892ad7d82c5438e76e57a44d21113#commitcomment-24177948
but currently I do not intend to do a complete overhaul
of the UEFI functionality in ReaR, cf. my
https://github.com/rear/rear/pull/1477#issuecomment-328472988
"I do not intend to do a complete overhaul"

OliverO2 commented at 2017-09-11 12:32:¶

@jsmeix OK. As I only have a limited amount of time available, I'll wait with testing until both known problems are addressed. I could also provide a PR with a complete UEFI-tested fix if that's what you'd prefer.

pcahyna commented at 2017-09-11 12:35:¶

@jsmeix with this commit you unfortunately broke it for the non-UEFI case, i.e. you reintroduced issue #1478

jsmeix commented at 2017-09-11 12:47:¶

@pcahyna
because my
https://github.com/rear/rear/commit/6e05b6f0e51fdfdf02c67edddb117b41a364c6f4
is based on the GitHub master code
it did not include anything from this pull request here
so that it cannot fix https://github.com/rear/rear/issues/1478
but my
https://github.com/rear/rear/commit/6e05b6f0e51fdfdf02c67edddb117b41a364c6f4
never was intended to fix https://github.com/rear/rear/issues/1478
it was only intended to make the existing code actually working
as it was menat to work - i.e. to fix only the particular reported
https://github.com/jsmeix/rear/commit/9bb07357ac7892ad7d82c5438e76e57a44d21113#commitcomment-24177948
and nothing more.

@OliverO2
I would very much appreciate it if you coud do a further
pull request with a complete UEFI-tested fix.
Many thanks in advance for it!
And no rush - we all have only limited amount of time available.

pcahyna commented at 2017-09-11 12:54:¶

@jsmeix I see. My point was that before your commit the master branch did not suffer from issue #1478 anymore and so this pull request was actually not needed. See https://github.com/rear/rear/pull/1477#issuecomment-328169575 (Arguably, this was purely coincidental and #1478 had not been fixed properly.) Anyway, we now need again to fix it.

jsmeix commented at 2017-09-12 08:23:¶

I close this pull request because it is
superseded by
https://github.com/rear/rear/pull/1481

[Export of Github issue for rear/rear.]

#1477 PR closed: Update 310_include_uefi_tools.sh¶