Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrected bad test for UEFI partition check inside /boot. #1257

Merged
merged 2 commits into from Mar 22, 2017

Conversation

gozora
Copy link
Member

@gozora gozora commented Mar 21, 2017

Today I've accidentally found a small regression that can basically avoid ReaR to correctly setting USING_UEFI_BOOTLOADER=1.

With old code:

if [[ -n $(find /boot -maxdepth 1 -iname efi -type d) ]]; then
    return    # not found
fi

If efi directory is found inside /boot 320_include_uefi_env.sh would prematurely return.

@gozora gozora requested review from gdha and jsmeix March 21, 2017 18:36
@gozora gozora self-assigned this Mar 21, 2017
@gozora gozora added the bug The code does not do what it is meant to do label Mar 21, 2017
@gdha
Copy link
Member

gdha commented Mar 21, 2017

@gozora To my knowledge this was already fixed - did you forgot a git pull?
See 80494f6#diff-be40da13c57e48c9ded13d219b62dff2

@gozora
Copy link
Member Author

gozora commented Mar 22, 2017

Hello @gdha,
My fix is in file 320_include_uefi_env.sh the one you are mentioning is about 310_include_uefi_tools.sh

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

I prefer to see the same test:
test "$( find /boot -maxdepth 1 -iname efi -type d )" || return as in script usr/share/rear/prep/default/310_include_uefi_tools.sh

@jsmeix
Copy link
Member

jsmeix commented Mar 22, 2017

@gozora
many thanks for finding the other faulty test!

Could you - by the way - have a look at my comment
in 310_include_uefi_tools.sh about whether or not it might
be better to abort with a clear Error message instead of
proceeding 'bona fide'?
My blind guess is that when the user has UEFI
those stuff in those scripts must be successfully done
and if that is not possible it should abort with an Error.
In particular because the 'prep' stage is run during
"rear mkrescue/mkbackup" where an Error abort can
tell the user early that something is wrong (in contrast
to an Error abort during "rear recover" where it is usually
too late for the user to get things properly fixed).
But I am not a sufficient UEFI expert to make an educated
decision if an Error abort is better here.

@jsmeix jsmeix added this to the ReaR v2.1 milestone Mar 22, 2017
Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

In
42d1c4c
it is o.k. for me.

@gozora
Copy link
Member Author

gozora commented Mar 22, 2017

@jsmeix

Could you - by the way - have a look at my comment
in 310_include_uefi_tools.sh about whether or not it might
be better to abort with a clear Error message instead of
proceeding 'bona fide'?
My blind guess is that when the user has UEFI
those stuff in those scripts must be successfully done
and if that is not possible it should abort with an Error.
In particular because the 'prep' stage is run during
"rear mkrescue/mkbackup" where an Error abort can
tell the user early that something is wrong (in contrast
to an Error abort during "rear recover" where it is usually
too late for the user to get things properly fixed).
But I am not a sufficient UEFI expert to make an educated
decision if an Error abort is better here.

Honestly? I really don't know which approach would be better :-(. I'll try to focus on this part of code during my testing and maybe to find out what would be better behavior ...

V.

@jsmeix
Copy link
Member

jsmeix commented Mar 22, 2017

@gozora
I fully agree with you.

First things first.

I will merge it soon because I think since your
42d1c4c
what @gdha requested is fulfilled.

@gdha gdha merged commit 19ec5cd into rear:master Mar 22, 2017
@jsmeix
Copy link
Member

jsmeix commented Mar 22, 2017

How unfortunate - now I can no longer merge it ;-)

@gdha
many thanks for your merge!
:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The code does not do what it is meant to do fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants