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
\$\endgroup\$
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.

\$\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.