Installing and removing kernel module RPMs

First, the script checks to see if it is installing two kernel module packages or removing them by looking at the first parameter. If no parameter is provided it complains and exits.

If installing, it checks if each of the packages even needs to be installed by looking at the version of the existing module. If they do need to be installed, install them; If they don't, simply say so and the rest of the script runs its course.

If I am removing them just run the command to do so.

The problem is, I have a lot of if...else...fi and if...elif...fi blocks setting two variables and then testing different combinations of the two. Even though the script works as is I feel like there should be a better way to do this.

I moved one block into a function even though this just put it somewhere else. It didn't actually clean it up at all.

```
#!/bin/bash

awk='/bin/awk'
enic_pkg="/home/$(logname)/kmod-enic-2.1.1.75-rhel6u5.el6.x86_64.rpm"
fnic_pkg="/home/$(logname)/kmod-fnic-1.6.0.12b-1.el6.x86_64.rpm"
modinfo='/sbin/modinfo'

if [ ! $1 ]; then
echo "Script must be executed with either 'install' or 'remove' as a paramter"
exit

else
method=$1

fi

test_ood() {
if [ ! $enic = "2.1.1.75" ]; then
enic_ood=true

else
echo "enic module is already up-to-date: $enic"

fi

if [ ! $fnic = "1.6.0.12b" ]; then
fnic_ood=true
else
echo "fnic module is already up-to-date: $fnic"

fi

}

if [ $method == "install" ]; then
yum='/usr/bin/yum -y localinstall'
enic=$(${modinfo} enic |grep "^version" |${awk} '{print $2}')
fnic=$(${modinfo} fnic |grep "^version" |${awk} '{print $2}')

test_ood

if [ $enic_ood -a $fnic_ood ]; then
echo "Both enic and fnic modules are out of date."
echo "enic: $enic"
echo "fnic: $fnic"
echo "Installing kmod-enic and kmod-fnic..."
${yum} $enic_pkg $fnic_pkg

elif [ $enic_ood -a ! $fnic_ood ]; then
echo "Only the enic module is out of date: $enic"

Should this be a shell script?

You mentioned that you wrote this script to push the packages to over 60 servers. If you have that many servers, then you should have a configuration management tool to do these kinds of tasks for you and much, much more. Typical solutions include Ansible, CFengine, Chef, and Puppet.

Instead of copying the RPMs to each machine, you should probably use a YUM repository, which may be one that you operate yourself, if those are self-built RPMs.

Version check

modinfo enic |grep "^version" |awk '{print $2}' would be better written as modinfo -F version enic 2>/dev/null. I recommend discarding error messages there, in case no such module is available.

However, I wonder why you bother querying modinfo at all, instead of just running yum and letting it automatically decide whether anything needs to be installed at all.

The conditionals are slightly redundant. You could just write

if [ $enic_ood -a $fnic_ood ]; then
    …
elif [ $enic_ood ]; then
    …
elif [ $fnic_ood ]; then
    …
else
    …
fi

I get the feeling, though, that nearly everything in the script is written twice. You could write a loop that handles both packages:

need_rpms=
for module in enic fnic ; do
    rpm=$HOME/kmod-$module-*.rpm
    want_version=$(rpm -qp --queryformat '%{VERSION}' $rpm)
    installed_version=$(/sbin/modinfo -F version "$module" 2>/dev/null)
    if [ "$want_version" = "$installed_version" ]; then
        echo "$module is already up-to-date: $installed_version"
    else
        need_rpms="$need_rpms $rpm"
    fi
done

if [ -n "$need_rpms" ]; then
    /usr/bin/yum -y localinstall $need_rpms
done

Alternatively, you could have one script that takes parameters, and run it once for each package.

Program structure

There should be an install() function and a remove() function.

Don't set yum='/usr/bin/yum -y localinstall' in one place and yum='/usr/bin/yum -y remove' in another place. I would expect it to be uniformly defined as yum='/usr/bin/yum -y', just like modinfo='/sbin/modinfo'.

This script always appears to succeed. Fatal errors should cause the script to exit with a non-zero status, and error messages should be printed to standard error. Exit statuses matter; even when executing the script over SSH, for example, the error status propagates all the way back to become the exit status of SSH.

install() {
    …
}

remove() {
    …
}

case "$1" in
  install|remove)
    "$1"
    ;;
  *)
    echo "$0 must be executed with either 'install' or 'remove'" >&2
    exit 1
    ;;
esac

StackExchange Code Review Q#93807, answer score: 5