Installing and removing kernel module RPMs

Question

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"
      echo "Installing kmod-enic..."
      ${yum} $enic_pkg

    elif [ ! $enic_ood -a $fnic_ood ]; then 
      echo "Only the fnic module is out of date: $fnic"
      echo "Installing kmod-fnic..."
      ${yum} $fnic_pkg

    else
      echo "Both modules are up-to-date. Nothing to do; exiting..."
      exit

    fi

    test_ood

elif [ $method == "remove" ];then
    yum='/usr/bin/yum -y remove'

    ${yum} kmod-enic kmod-fnic

else
    echo "Unrecognized option: $method"
    exit

fi

I was thinking I could make the $method if tests case switches and put the code for each of those if blocks into methods of their own. That would at least eliminate the outer if...elif...fi block. Although, I'd only be converting it to another type of block.

Answer 1

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

Answer 2

In addition to what @200_success said.

Avoid hard-coding the target version numbers in multiple places. Put them in variables at the top of the file, and use them to derive all other uses.

The if-elif-else conditions are tedious because it seems the script is trying to be "too nice". Treating all permutations of installing these two packages or not is too much. I would simplify to treat each package individually, and simply print if they will be installed or not. No need to treat them together with words like "both" or "only".

Lastly, a very minor thing, I'm not a fan of unnecessary quotes. In all these statements the quotes are safe to drop:

awk='/bin/awk'
modinfo='/sbin/modinfo'

if [ ! $enic = "2.1.1.75" ]; then ....

if [ $method == "install" ]; then ....