4
\$\begingroup\$

I made this script when I started downloading the latest stable linux kernel's source from https://www.kernel.org to compile it myself, now I have improved it a bit and decided to create a public repo for it on github (https://github.com/Tom1380/latestkernel).

This is the code (mind you this is the first time I made bash scripts):

lk (stands for latest kernel):

#!/usr/bin/env bash

cd ~

if [ ! -d "linux-stable" ]; then
    mkdir linux-stable
fi

if [ ! -f "/etc/latest_kernel" ]; then
    if  [[ $EUID > 0 ]] ; then
        echo "/etc/latest_kernel does not exist and you are not running the script as root, " \
              "please fix that either by manually running 'sudo create_etc_lk', " \
              "or rerun the script as root to automatically fix it."
        exit
    else
        touch /etc/latest_kernel
        echo "$(uname -r)" > /etc/latest_kernel
    fi
fi

wget=$(wget --output-document - --quiet https://www.kernel.org/ | grep -A 1 "latest_link")

wget=${wget##*.tar.xz\">}

wget=${wget%</a>}

latest_acknowledged=$(</etc/latest_kernel)

if [ "$wget" == "$latest_acknowledged" ]; then
    echo "You have the latest stable kernel downloaded: $wget"
#    Uncomment (take out the #) the first 4 lines below if you want to be prompted when you are using a different kernel than
#    the latest one while already having installed it
#    runningkernel=$(uname -r)
#    if [ $runningkernel != $latest_acknowledged ]; then
#        echo "But you are running $runningkernel"
#    fi
    exit
fi

echo "Updated kernel available: $wget, you have $latest_acknowledged."

if  [[ $EUID > 0 ]] ; then
    echo "If you wish to download the latest kernel, rerun the script as root."
    exit
fi

# Since there is only a '>', /etc/latest_kernel will be overwritten entirely.

echo "$wget" > /etc/latest_kernel

echo "Writing latest kernel available in /etc/latest_kernel."

echo "Preparing to parse link to latest kernel for wget."

wget=$(wget --output-document - --quiet https://www.kernel.org/ | grep -A 1 "latest_link")

wget=${wget##*<a href=\"}

wget=${wget%\">*}

echo "Done parsing."

cd ~/linux-stable

echo "Changed cwd to ~/linux-stable to download kernel source."

echo "Downloading, this may take up to 10 minutes."

wget $wget

echo "Finished downloading..."

echo "Uncompressing the kernel's source."

tar xvfJ linux-$(</etc/latest_kernel).tar.xz

echo "Done uncompressing the kernel's source."

rm linux-$(</etc/latest_kernel).tar.xz

echo "Done removing the old archive, end of the script."

create_etc_lk:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

touch /etc/latest_kernel && echo "$(uname -r)" > /etc/latest_kernel

install:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

cp lk /usr/bin/lk
cp create_etc_lk /usr/bin/create_etc_lk
cp purgelatestkernel /usr/bin/purgelatestkernel

purgelatestkernel:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

cp lk /usr/bin/lk
cp create_etc_lk /usr/bin/create_etc_lk
cp purgelatestkernel /usr/bin/purgelatestkernel

PS: Feel free to clone the repo if you think lk might be helpful to you

\$\endgroup\$
2
\$\begingroup\$

Good stuff:

  • Yay for putting your code on github. I see a lot of folks afraid to wait until everything is perfect to do that. But having the backup and history of git is too handy to wait like that. So I love seeing somebody doing that from day 1 of a project.
  • Yay for use the double square bracket form of if and checking for common mistakes.
  • Yay for using env. As you can see from my github I'm not so great at this one, but it is a good idea.

Suggestions:

  • Move common code into a library. Your EUID check is the most obvious culprit. To create a shell library you should create a file of functions and then source it into each of your commands. So to translate your check into a function is easy:

    function validate_root {
        if [[ $EUID > 0 ]] ; then
                echo "You need root privileges for this script."
                exit
        fi
    }
    

    Then use the library later like so:

    source lib.sh
    
    validate_root # be root or exit
    
  • Move common data into the library. The most common item is /etc/latest-kernel... what if you want to move it somewhere else? If it is in a variable in one place it will be a lot easier to do. There's no reason not to throw those into the common shell library file too.

  • It looks like you made a cut and paste error on purgelatestkernel but since I can check out the github version I can see what you meant there. Generally you're not supposed to edit code during code review put I think you'd be ok to fix that part up since you're not altering the code you meant to review.
  • Avoid duplicate code by looping in the install:

    for file in lk create_etc_lk purgelatestkernel
    do
            echo copying $file to /usr/bin
            cp $file /usr/bin
    done
    

    This also makes it easy to add other things like a validation of permissions or check for return values without insane amounts of error-prone cut and paste.

  • Instead of having the option for a prompt be something that requires a source code alteration why not make it conditional based on a variable that possibly could be altered with a command line option?
\$\endgroup\$
  • \$\begingroup\$ Dude I had no idea bash had something similar to imports! This is gold! Thanks, I'm editing the files right now and pushing them to the repo. \$\endgroup\$ – Tommaso Thea Cioni May 15 '17 at 11:34
1
\$\begingroup\$

Exit code

When a script fails, it should exit with a non-zero exit code, so that other scripts using it could respond accordingly, take appropriate actions or clean up.

For example, it's a good idea to exit with non-zero in the root user validation part:

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit 1
fi

Download once and reuse

The script downloads https://www.kernel.org/ twice, and then extracts different parts from it. That's unnecessary. You could download once and extract the different parts into different variables. And those variables could reflect their purpose better, for example version and url, respectively.

Delay filesystem writes

The script creates the directory ~/linux-stable up front, before doing anything else. It would be better to delay that until the directory is actually needed.

Also, instead of checking if the directory doesn't exist, you could write more compactly using the -p flag of mkdir.

mkdir -p linux-stable

Pointless touch

You could drop the touch statement here:

touch /etc/latest_kernel
echo "$(uname -r)" > /etc/latest_kernel
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.