2
\$\begingroup\$

This is my first bigger than a few lines bash script.

It automates kernel updating and removing on Ubuntu through ukuu (Ubuntu kernel update utility).

I'm not yet familiar with all the bells and whistles of GNU utilities and want to strive for clarity. The only thing I know now is grep -P and everything seems like a nail if you have a hammer. Also this is mostly stitched from examples on StackExchange.

What would you change to make the code more readable or perhaps better in some other ways?

#!/bin/bash
updateInstalledKernels () { # TODO (low priority) modify to not double-count kernels which have two "Installed" records in ukuu --list
 while read I
 do
 INSTALLED_KERNELS=( "${INSTALLED_KERNELS[@]}" "$I" ) 
 done < <(printf "$AVAILABLE_KERNELS" | grep -oP 'v\d+\.\d+((\.\d+)|(-\w+))*(?=\s+[0-9.]*\s+(Installed|Running)\s*?\z)')
}
containsElement () { # usage: containsElement string element1 element2 ...
 local e match="1ドル"
 shift
 for e; do [[ "$e" == "$match" ]] && return 0; done
 return 1
}
FALLBACK_KERNELS=( 'v4.15.0-33.36' 'v4.15.0-30.32' ) # array of kernels for fallback
echo "fallback kernels: ${FALLBACK_KERNELS[@]}"
# TODO variable which chooses the number of latest kernels to keep
AVAILABLE_KERNELS=$(ukuu --list)
if ! echo "$AVAILABLE_KERNELS" | grep -oPzq 'Available Kernels.*?\n=+\n\K.*?(Installed|Running).*?\n';
then
 ukuu --install-latest
else
 LATEST_KERNEL=$(echo "$AVAILABLE_KERNELS" | grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)')
 echo "Latest kernel ${LATEST_KERNEL} installed. Removing old kernels apart from ${FALLBACK_KERNELS[@]}."
 declare -a INSTALLED_KERNELS
 updateInstalledKernels
 echo "Installed kernels: ${INSTALLED_KERNELS[@]}."
 for INSTALLED_KERNEL in "${INSTALLED_KERNELS[@]}"
 do
 echo "$INSTALLED_KERNEL"
 if ! containsElement "$INSTALLED_KERNEL" "${FALLBACK_KERNELS[@]}"
 then
 if ! echo "$INSTALLED_KERNEL" | grep -q "$LATEST_KERNEL"
 then
 echo "${INSTALLED_KERNEL} is not fallback, nor latest. Removing it..."
 ukuu --remove "$INSTALLED_KERNEL"
 fi
 fi
 done <<< "$INSTALLED_KERNELS"
fi
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Oct 5, 2018 at 19:31
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

The only thing I know now is grep -P and everything seems like a nail if you have a hammer.

I hope that's an exaggeration. Otherwise, I don't know how you will understand a review ;-)

Use more here-strings

The script uses here-strings in only one place, when there are more places where it would be good to use. For example, instead of this:

LATEST_KERNEL=$(echo "$AVAILABLE_KERNELS" | grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)')

This would be better, and eliminate an unnecessary echo call:

LATEST_KERNEL=$(grep -oPz 'Available Kernels.*?\n=+\n\Kv.+?(?=\s.*?\n)' <<< "$AVAILABLE_KERNELS")

Eliminate unnecessary condition

The inner if here can be eliminated:

if ! containsElement "$INSTALLED_KERNEL" "${FALLBACK_KERNELS[@]}"
then
 if ! echo "$INSTALLED_KERNEL" | grep -q "$LATEST_KERNEL"
 then
 echo "${INSTALLED_KERNEL} is not fallback, nor latest. Removing it..."
 ukuu --remove "$INSTALLED_KERNEL"
 fi
fi

You could add "$INSTALLED_KERNEL" to the parameter list of the call to containsElement. Not only the code will be simpler, but it will also eliminate a grep call. (And the echo with the grep, which should have been a here-string anyway, as explained earlier.)

Understand the flags of commands

This call looks strange: grep -oPzq .... Because of the -q. Thanks to the -q, the command will produce no output. Which makes the -o flag unnecessary. I suggest to review in man grep all the flags you're using, understand what they do, and consistently eliminate unnecessary flags.

Naming variables

I and e are too short, they don't help understanding their purpose, and therefore the code.

All capital letters are traditionally used for system variables. For all local variables in shell scripts, it's better to use lowercase names to avoid any confusion.

Appending to an array

This statement appends a value to the INSTALLED_KERNELS array:

INSTALLED_KERNELS=( "${INSTALLED_KERNELS[@]}" "$I" )

A simpler way to achieve the same:

INSTALLED_KERNELS+=("$I")

Declare important global variables and constants at the top

Some of the variables are crucial for the behavior of the script, for example FALLBACK_KERNELS. What if later you want to change the fallback kernels? You have to read through the script to find the declaration in the middle. It would be a lot easier if this was declared at the top, so you would not need to re-read and re-understand what's going on just to make a simple change.

answered Oct 18, 2018 at 12:39
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.