4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a reason you want to write a script instead of just running yum on the command line? \$\endgroup\$ – 200_success Jun 17 '15 at 4:17
  • 1
    \$\begingroup\$ It needs to be done on 60 to 70 servers. We're pushing it out and executing remotely. \$\endgroup\$ – theillien Jun 17 '15 at 4:47
5
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ CM is a bit of a sore spot. I've been trying to get the powers that be to allow me to install Puppet for well over three years. As for allowing yum to handle what it can, we currently have everything set up to use the Red Hat-provided modules which are steps behind what we need. They are essentially just the modules that come with the kernel so yum would assume it has to install the package anyway so if a server somehow managed to have the version it needs (not a stretch in this environment), it would be installing over it which is a bit unnecessary. \$\endgroup\$ – theillien Jun 17 '15 at 6:49
  • \$\begingroup\$ Normally, I would upload any packages to our Satellite and allow yum to pull from there. Unfortunately, due to bureaucratic muckety-muck we have been stuck with an expired Satellite certificate with no ETA on when we'll be able to put it back into use. As a result, we're left copying the files to each server. Technically, we could set up another repo somewhere else, but that would involve configuring each server to use it anyway which doesn't really save anything. \$\endgroup\$ – theillien Jun 17 '15 at 6:51
  • \$\begingroup\$ I do like that modinfo has that -F option. I don't know why it never occurs to me that commands have the means to provide specific information. Thank you for your input. I'll definitely be putting it to use. \$\endgroup\$ – theillien Jun 17 '15 at 6:54
1
\$\begingroup\$

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 ....
| improve this answer | |
\$\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.