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

Basic support for EFISTUB booting. #2030

Merged
merged 3 commits into from Feb 11, 2019
Merged

Conversation

gozora
Copy link
Member

@gozora gozora commented Feb 4, 2019

Pull Request Details:
  • Type: New Feature

  • Impact: Normal

  • Reference to related issue (URL): Support EFISTUB booting (e.g. on Arch Linux) #1942

  • How was this pull request tested?
    Full backup restore on Arch Linux configured for EFISTUB booting.
    Migration from GRUB2 to EFISTUB on SLES2 SP2.

  • Brief description of the changes in this pull request:
    This patch enables basic support in ReaR for systems setup to boot using EFISTUB.

@gozora gozora added the enhancement Adaptions and new features label Feb 4, 2019
@gozora gozora added this to the ReaR v2.5 milestone Feb 4, 2019
@gozora gozora self-assigned this Feb 4, 2019
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.

@gozora great work - well done Vlad!

(*p) Disk=${Disk%p} ;;
(*-part) Disk=${Disk%-part} ;;
(*_part) Disk=${Disk%_part} ;;
(*) Log "Unsupported kpartx partition delimiter for $Dev"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't an "Unsupported kpartx partition delimiter for $Dev"
perhaps even an error that should be made visible to the user
(without aborting rear recover in its late finalize stage) via

LogPrintError "Unsupported kpartx partition delimiter for $Dev"

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, this was just a copy, paste from usr/share/rear/finalize/Linux-i386/630_run_efibootmgr.sh ;-).

But I can change it to LogPrintError in both files if needed ...

Copy link
Member

Choose a reason for hiding this comment

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

Fortunately this particular lines of code are not from me :-)

From plain looking at the code in both files
I think it is a real error in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, looks like error to me as well...
To have better tracking, I'll open separate PR for this "bug" once #2030 is merged.

else
Log "EFI_STUB: Kernel file: $KERNEL_FILE is hosted on vfat filesystem"
fi

Copy link
Member

Choose a reason for hiding this comment

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

Ony a side note:
You could - if you like - simplify it e.g. as

if ERROR_CONDITION ; then
    Error "..."
fi
Log "..."

or even

ERROR_CONDITION && Error "..."
Log "..."

Copy link
Member Author

Choose a reason for hiding this comment

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

Something like:

[[ "$uefi_fs_type" != "vfat" ]] && Error "EFI_STUB: Kernel file: $KERNEL_FILE is not hosted on vfat filesystem"
Log "EFI_STUB: Kernel file: $KERNEL_FILE is hosted on vfat filesystem"

?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - but only if you like it - I see such kind of code e.g. in
usr/share/rear/layout/save/GNU/Linux/190_opaldisk_layout.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

In this particular case I don't have any real preference, so I'll change it as agreed ...

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.

From plain looking at the code all looks o.k.

Many thanks for that great new feature
which also supports a clean way to migrate to EFISTUB booting.

….conf instead of hidden in code.

Simplified condition syntax in prep/GNU/Linux/500_EFISTUB_check_kernel.sh.
@gozora
Copy link
Member Author

gozora commented Feb 11, 2019

If there are no further objections, I'll merge this code later today.

V.

@gozora gozora merged commit 4e2a181 into rear:master Feb 11, 2019
@gozora gozora deleted the EFISTUB_upstream_merge branch February 23, 2019 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants