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

Fix disk device name in efibootmgr call for eMMC devices #2104

Merged
merged 2 commits into from Apr 3, 2019

Conversation

fabz5
Copy link
Contributor

@fabz5 fabz5 commented Mar 29, 2019

Pull Request Details:
  • Type: Bug Fix / New Feature / Enhancement / Other?
    Enhancement

  • Impact: Low / Normal / High / Critical / Urgent
    Low

  • Reference to related issue (URL):
    rear recover fails to create UEFI boot entry for eMMC device #2103

  • How was this pull request tested?
    Manually on corresponding hardware (Z83-F Mini PC)

  • Brief description of the changes in this pull request:
    For eMMC devices strip the trailing "p" in the disk device name,
    so that during recovery the efibootmgr call will not fail, but create an UEFI boot entry.

@jsmeix jsmeix self-assigned this Apr 1, 2019
@jsmeix jsmeix added the enhancement Adaptions and new features label Apr 1, 2019
@jsmeix jsmeix added this to the ReaR v2.5 milestone Apr 1, 2019
@jsmeix
Copy link
Member

jsmeix commented Apr 1, 2019

@fabz5
from plain looking at the code your changes look o.k. to me.

But currently it looks as if your additional conditional code
somehow belongs to the have 'mapper' in devname case.
Technically the elif makes it separated but
it doesn't make it look separated,
cf. "Code must be easy to read" in
https://github.com/rear/rear/wiki/Coding-Style

Therefore I suggest this changed code

# /dev/sda or /dev/mapper/vol34_part or /dev/mapper/mpath99p or /dev/mmcblk0p
Disk=$( echo ${Dev%$ParNr} )

# Strip trailing partition remainders like '_part' or '-part' or 'p'
# if we have 'mapper' in disk device name:
if [[ ${Dev/mapper//} != $Dev ]] ; then
    # we only expect mpath_partX  or mpathpX or mpath-partX
    case $Disk in
        (*p)     Disk=${Disk%p} ;;
        (*-part) Disk=${Disk%-part} ;;
        (*_part) Disk=${Disk%_part} ;;
        (*)      Log "Unsupported kpartx partition delimiter for $Dev"
    esac
fi

# For eMMC devices the trailing 'p' in the Disk value like /dev/mmcblk0p
# needs to be stripped, otherwise the efibootmgr call will fail
# because of a wrong disk device name. See also
# https://github.com/rear/rear/issues/2103
if [[ $Disk = *'/mmcblk'+([0-9])p ]] ; then
    Disk=${Disk%p}
fi

to keep the eMMC devices case more clearly separtated
from the 'mapper' cases and have the /dev/mmcblk0p also
in the comment at Disk=$( echo ${Dev%$ParNr} )
plus a better readable comment for the 'mapper' case.

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 (I don't have a eMMC device)
your changes look o.k. to me so I approve it.

But I would appreciate it if you could have a look at my proposal in
#2104 (comment)
if you could implement it this way - provided it still works then.

@fabz5
Copy link
Contributor Author

fabz5 commented Apr 2, 2019

You are right of course, the coding was a bit sloppy. I guess it was too late at night when I did this. I will test your suggested changes and then recommit.

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.

Now in this pull request there are also changes for
usr/share/rear/build/GNU/Linux/630_verify_resolv_conf_file.sh
which should not be part of this pull request.

@fabz5
Copy link
Contributor Author

fabz5 commented Apr 2, 2019

(Facepalm) Sorry. This is not my day. I will fix this.

@fabz5
Copy link
Contributor Author

fabz5 commented Apr 2, 2019

After learning how to remove commits from pull requests I hope now it is fine.

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.

Now it looks good to me.

@jsmeix jsmeix requested a review from a team April 3, 2019 07:42
@jsmeix
Copy link
Member

jsmeix commented Apr 3, 2019

@rear/contributors
if there are no objections I would like to merge it today afternoon.

@jsmeix jsmeix merged commit 6dde94a into rear:master Apr 3, 2019
@jsmeix
Copy link
Member

jsmeix commented Apr 3, 2019

@fabz5
thank you for your contribution that improves support of eMMC devices in ReaR.

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

2 participants