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.

Sign up for GitHub

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

Jump to bottom

check kernel version and issue warning if necessary #180

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

check kernel version and issue warning if necessary #180

Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
61b193c
check kernel version and issue warning if necessary
shrx Jun 23, 2015
c478341
check kernel version and issue warning if necessary
shrx Jun 24, 2015
47f760c
Update rpi-update
shrx May 19, 2016
18dea0d
Update rpi-update
shrx May 19, 2016
4e5fe1f
Update rpi-update
shrx May 19, 2016
986365d
Update rpi-update
shrx May 19, 2016
6d43989
Update rpi-update
shrx May 19, 2016
9856b77
Update rpi-update
shrx May 26, 2016
21b75dd
Update rpi-update
shrx Jul 26, 2016
f5b32bf
Update rpi-update
shrx Jul 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions rpi-update
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,33 @@ function update_sdk {
}

function show_notice {
local NOTICE=`curl -Lfs https://raw.githubusercontent.com/hexxeh/rpi-firmware/${FW_REV}/NOTICE.md`
local FULL_NOTICE=`curl -Lfs https://raw.githubusercontent.com/hexxeh/rpi-firmware/${FW_REV}/NOTICE.md`
local NOTICE=$(echo "$FULL_NOTICE" | tail -n+2)
if [ -z "$NOTICE" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be checking $FULL_NOTICE instead of $NOTICE ?
I think as it is currently, it wouldn't display the NOTICE if it was only one line long?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I think this line should be if [ -z "$FULL_NOTICE" ]; then and the local NOTICE=$(echo "$FULL_NOTICE" | tail -n+2) can probably be moved down to line 138 or so.

return
fi
local NOTICE_HASH_HEAD=$(echo "$FULL_NOTICE" | head -1)
if [ $(echo "${NOTICE_HASH_HEAD}" | awk -F: '{print $1}') == "HASH" ]; then
local NOTICE_HASH_EXISTS=true
Copy link
Contributor

Choose a reason for hiding this comment

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

For style-consistency with the rest of the code in this script, you should probably use 1 and 0 instead of true and false, and then change all your conditionals to e.g. if [[ ${NOTICE_HASH_EXISTS} -eq 1 ]]; then

local NOTICE_HASH=$(echo "${NOTICE_HASH_HEAD}" | awk '{print $2}')
else
local NOTICE_HASH_EXISTS=false
fi
if [ -f "$FW_REVFILE" ]; then
local LOCAL_HASH=$(cat "$FW_REVFILE")
else
local LOCAL_HASH=0
fi
if ${NOTICE_HASH_EXISTS}; then
local NEW_HASH=${FW_REV}
local LOCAL_lt_NOTICE=$(compare_hashes ${LOCAL_HASH} lt ${NOTICE_HASH})
local NEW_ge_NOTICE=$(compare_hashes ${NEW_HASH} ge ${NOTICE_HASH})
Copy link
Contributor

Choose a reason for hiding this comment

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

By calling get_hash_date inside compare_hashes, this results in four separate requests to curl.
If you rearranged the logic slightly, and got the hash dates before comparing them, then you'd only need three requests to curl.
I guess there's also no need to call get_hash_date in the case when LOCAL_HASH has been set to 0.
And is it possible that any of LOCAL_HASH, NOTICE_HASH or NEW_HASH could have the same value? That might allow you to (conditionally) eliminate another call to curl.

if ! ${LOCAL_lt_NOTICE} && ! ${NEW_ge_NOTICE}; then
return
fi
else
NOTICE="$FULL_NOTICE"
fi
echo "$NOTICE"
if ! echo "$NOTICE" | grep -q WARNING; then
return
Expand Down Expand Up @@ -281,6 +304,21 @@ function noobs_fix {

}

function get_hash_date {
commit_api="https://api.github.com/repos/Hexxeh/rpi-firmware/git/commits/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for style-consistency this should be upper-cased to COMMIT_API, and it should be dynamically calculated from $REPO_URI in the same way that REPO_API is on line 364; in case the user supplies a custom REPO_URI.

In fact, perhaps it makes sense to have REPO_API=${REPO_URI/github.com/api.github.com\/repos} up at the top of the script, and then line 315 could use $REPO_API/git/commits/$1 and line 365 etc. could use $REPO_API/git/refs/heads/${BRANCH}

Copy link
Contributor

Choose a reason for hiding this comment

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

I've made this change in #279 🙂

curl -Ls "$commit_api"$1 | grep "date" | head -1 | awk -F\" '{print $4}'
}

function compare_hashes {
date1=$(get_hash_date $1)
date2=$(get_hash_date $3)
if [ $(date -d "$date1" +%s) -$2 $(date -d "$date2" +%s) ]; then
echo true
else
echo false
fi
}

if [[ ${EUID} -ne 0 ]]; then
echo " !!! This tool must be run as root"
exit 1
Expand Down Expand Up @@ -320,13 +358,13 @@ command -v readelf >/dev/null 2>&1 || {

# ask github for latest version hash
REPO_API=${REPO_URI/github.com/api.github.com\/repos}/git/refs/heads/${BRANCH}
GITREV=$(curl -s ${REPO_API} | awk '{ if ($1 == "\"sha\":") { print substr($2, 2, 40) } }')
GITREV=$(curl -Ls ${REPO_API} | awk '{ if ($1 == "\"sha\":") { print substr($2, 2, 40) } }')
FW_REV=${FW_REV:-${GITREV}}

if [[ "${FW_REV}" == "" ]]; then
echo " *** No hash received from github: ${REPO_API}"
# run again with errors not suppressed
curl ${REPO_API}
curl -L ${REPO_API}
exit 1
fi

Expand Down