Update 310_include_uefi_tools.sh #1477

jmazanek · 2017-09-08T11:14:45Z

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

jmazanek · 2017-09-08T11:17:12Z

Related issue: #1478

jsmeix · 2017-09-08T11:47:00Z

@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".

pcahyna · 2017-09-08T11:47:58Z

usr/share/rear/prep/default/310_include_uefi_tools.sh

@@ -8,7 +8,9 @@ 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
+    if ! grep -q '/boot/efi' /proc/mounts; then


I think that using mountpoint -q /boot/efi is more elegant.

or rather mountpoint -q /boot/[Ee][Ff][Ii] as we want to be case-insensitive.

gozora · 2017-09-08T11:51:09Z

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 · 2017-09-08T12:03:03Z

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 · 2017-09-08T12:13:21Z

@gozora cf.
9bb0735
and for more check
"git log -p --follow usr/share/rear/prep/default/310_include_uefi_tools.sh"

pcahyna · 2017-09-08T12:19:36Z

usr/share/rear/prep/default/310_include_uefi_tools.sh

@@ -8,7 +8,9 @@ if [[ ! -d /boot/[eE][fF][iI] ]]; then
    if is_true $USING_UEFI_BOOTLOADER; then


Given that the whole block is executed when the directory does not exist, what is the point of checking whether it is a mount point?
Shouldn't it be rather

if [[ ! -d /boot/[eE][fF][iI] || ! mountpoint -q /boot/[eE][fF][iI] ]]; then

?

@pcahyna , correct . I have edited the code

Sorry this is not correct either, i will provide a better fix later today.

pcahyna · 2017-09-08T17:44:57Z

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

OliverO2 · 2017-09-08T20:20:28Z

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 · 2017-09-08T20:35:15Z

@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 jsmeix@9bb0735#commitcomment-24177948 on 9bb0735 (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 · 2017-09-11T09:26:15Z

With
6e05b6f
I use simpler code in 310_include_uefi_tools.sh to fix
jsmeix@9bb0735#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 · 2017-09-11T10:43:47Z

@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 · 2017-09-11T11:47:37Z

@OliverO2
my immediate
6e05b6f
was only meant to fix your particular reported
jsmeix@9bb0735#commitcomment-24177948
but currently I do not intend to do a complete overhaul
of the UEFI functionality in ReaR, cf. my
#1477 (comment)
"I do not intend to do a complete overhaul"

OliverO2 · 2017-09-11T12:32:04Z

@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 · 2017-09-11T12:35:12Z

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

jsmeix · 2017-09-11T12:47:29Z

@pcahyna
because my
6e05b6f
is based on the GitHub master code
it did not include anything from this pull request here
so that it cannot fix #1478
but my
6e05b6f
never was intended to fix #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
jsmeix@9bb0735#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 · 2017-09-11T12:54:41Z

@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 #1477 (comment) (Arguably, this was purely coincidental and #1478 had not been fixed properly.) Anyway, we now need again to fix it.

…1478)

jsmeix · 2017-09-12T08:23:42Z

I close this pull request because it is
superseded by
#1481

Update 310_include_uefi_tools.sh

7c880ed

jmazanek mentioned this pull request Sep 8, 2017

Rear terminates if the /boot/efi is not mounted #1478

Closed

jsmeix requested review from gdha and gozora September 8, 2017 11:44

pcahyna reviewed Sep 8, 2017

View reviewed changes

Update 310_include_uefi_tools.sh

8e6b797

OliverO2 added a commit to OliverO2/rear that referenced this pull request Sep 11, 2017

Fix UEFI tools integration (should address issues in rear#1477, rear#…

cb03304

…1478)

OliverO2 mentioned this pull request Sep 11, 2017

Fix UEFI tools integration (should address issues in #1477, #1478) #1481

Merged

jsmeix self-assigned this Sep 12, 2017

jsmeix added the won't fix / can't fix / obsolete label Sep 12, 2017

jsmeix closed this Sep 12, 2017

jsmeix removed request for gdha and gozora September 12, 2017 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update 310_include_uefi_tools.sh #1477

Update 310_include_uefi_tools.sh #1477

jmazanek commented Sep 8, 2017

jmazanek commented Sep 8, 2017

jsmeix commented Sep 8, 2017

pcahyna Sep 8, 2017

pcahyna Sep 8, 2017

gozora commented Sep 8, 2017

gozora commented Sep 8, 2017

jsmeix commented Sep 8, 2017 •

edited

pcahyna Sep 8, 2017

jmazanek Sep 8, 2017

pcahyna Sep 8, 2017

pcahyna commented Sep 8, 2017

OliverO2 commented Sep 8, 2017 •

edited

pcahyna commented Sep 8, 2017 •

edited

jsmeix commented Sep 11, 2017 •

edited

OliverO2 commented Sep 11, 2017 •

edited

jsmeix commented Sep 11, 2017

OliverO2 commented Sep 11, 2017

pcahyna commented Sep 11, 2017

jsmeix commented Sep 11, 2017

pcahyna commented Sep 11, 2017

jsmeix commented Sep 12, 2017

		@@ -8,7 +8,9 @@ if [[ ! -d /boot/[eE][fF][iI] ]]; then
		if is_true $USING_UEFI_BOOTLOADER; then

Update 310_include_uefi_tools.sh #1477

Update 310_include_uefi_tools.sh #1477

Conversation

jmazanek commented Sep 8, 2017

jmazanek commented Sep 8, 2017

jsmeix commented Sep 8, 2017

pcahyna Sep 8, 2017

Choose a reason for hiding this comment

pcahyna Sep 8, 2017

Choose a reason for hiding this comment

gozora commented Sep 8, 2017

gozora commented Sep 8, 2017

jsmeix commented Sep 8, 2017 • edited

pcahyna Sep 8, 2017

Choose a reason for hiding this comment

jmazanek Sep 8, 2017

Choose a reason for hiding this comment

pcahyna Sep 8, 2017

Choose a reason for hiding this comment

pcahyna commented Sep 8, 2017

OliverO2 commented Sep 8, 2017 • edited

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

2. USING_UEFI_BOOTLOADER is used before initalization

pcahyna commented Sep 8, 2017 • edited

jsmeix commented Sep 11, 2017 • edited

OliverO2 commented Sep 11, 2017 • edited

jsmeix commented Sep 11, 2017

OliverO2 commented Sep 11, 2017

pcahyna commented Sep 11, 2017

jsmeix commented Sep 11, 2017

pcahyna commented Sep 11, 2017

jsmeix commented Sep 12, 2017

jsmeix commented Sep 8, 2017 •

edited

OliverO2 commented Sep 8, 2017 •

edited

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

2. `USING_UEFI_BOOTLOADER` is used before initalization

pcahyna commented Sep 8, 2017 •

edited

jsmeix commented Sep 11, 2017 •

edited

OliverO2 commented Sep 11, 2017 •

edited