Skip to content

Commit

Permalink
310_include_uefi_tools.sh : fix the test for /boot/{efi|EFI} - see is…
Browse files Browse the repository at this point in the history 
…sue #1239 and #1225
  • Loading branch information
@gdha
gdha committed Mar 14, 2017
1 parent 3c67663 commit 6c477d4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion usr/share/rear/prep/default/310_include_uefi_tools.sh
View file
Expand Up @@ -6,7 +6,7 @@ if grep -qw 'noefi' /proc/cmdline; then
fi

# next step, is checking /boot/efi directory case-insensitive for the /EFI part (we need it)
if [[ -n $(find /boot -maxdepth 1 -iname efi -type d) ]]; then
if [[ $(find /boot -maxdepth 1 -iname efi -type d | wc -l) -eq 0 ]] ; then
return # must be mounted
fi

Expand Down

6 comments on commit 6c477d4

@jsmeix
Copy link
Member

@jsmeix jsmeix commented on 6c477d4 Mar 15, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for the complicated "| wc -l) -eq 0".

The root cause was missing quoting because
plain 'test -n' without argument results 'true':

# test -n && echo y || echo n
y

# test -n "" && echo y || echo n
n

# find /boot -maxdepth 1 -iname efi -type d && echo y || echo n
y

# test -n $( find /boot -maxdepth 1 -iname efi -type d ) && echo y || echo n
y

# test -n "$( find /boot -maxdepth 1 -iname efi -type d )" && echo y || echo n
n

Cf.
usr/share/rear/rescue/default/850_save_sysfs_uefi_vars.sh

    # Double quotes are mandatory here because 'test -f' without any (possibly empty) argument results true:
    test -f "$UEFI_BOOTLOADER" && continue

@jsmeix
Copy link
Member

@jsmeix jsmeix commented on 6c477d4 Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference: Also see
#1239

@jsmeix
Copy link
Member

@jsmeix jsmeix commented on 6c477d4 Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand the logic:

According to
git log -p --follow usr/share/rear/prep/default/310_include_uefi_tools.sh
it was initially

# next step, is checking /boot/efi directory (we need it)
if [[ ! -d /boot/efi ]]; then
    return    # must be mounted
fi

which I understand:
"return when there is no /boot/efi directory".

This was changed by @ProBackup-nl to

# next step, is checking /boot/efi directory case-insensitive for the /EFI part (we need it)
if [[ -n $(find /boot -maxdepth 1 -iname efi -type d) ]]; then
    return    # must be mounted
fi

which I do not understand because as far as I see this means:
"return if a /boot/[eE][fF][iI] directory can be found"
(find results a non-empty string which means it found something).
I think this is inverted logic compared to what it was before
or am I somehow blind and do not see how it actually works?

@jsmeix
Copy link
Member

@jsmeix jsmeix commented on 6c477d4 Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did
#1246
a proposal how I think the test for
the /boot/efi directory might be simplified.

FYI:
Plain 'test' without anything results false:

# test && echo y || echo n
n

in contrast to 'test -n' without argument for '-n':

# test -n && echo y || echo n
y

at least on my SLES11 SP3 system.

@ProBackup-nl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsmeis I think I did not change the logic (except for the quoting mistake).

original:
if [[ ! -d /boot/efi ]]; then
If no /boot/efi directory exists then... return

new:
if [[ -n "$(find /boot -maxdepth 1 -iname efi -type d)" ]]; then
If there is no /boot (case sensitive) /efi (case insensitive) directory then return.

The "directory not exists" logic ! -d has been replaced by: find result is empty: -n "$(find)".

@jsmeix
Copy link
Member

@jsmeix jsmeix commented on 6c477d4 Mar 17, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But 'test -n' tests for non empty, i.e.
test -n "$( find ... )" is true when "$( find ... )" is not empty.

Please sign in to comment.