HiveBrain v1.2.0
Get Started
← Back to all entries
patternshellMinor

Check disk space against a threshold

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
spacediskagainstthresholdcheck

Problem

The below script is one I made to check the disk space on mounted partitions under Debian Wheezy. I tried adding a -d switch to force printing out the used data, but its main purpose is to run as a cron job, and email the administrator if any disks are below a certain threshold.

What I'm looking for is to make sure it's POSIX compliant (in case it needs to be called by another script/application), and that it's rock solid. It works on my servers as is, with and without the -d. It requires that if an argument is given, it's a mount point. I am trying to get the proper coding practices done properly, as well.

#!/bin/sh

MOUNTPOINTS="/ /var /home" #The mount points to always check; the user can
                           #specify more via the command line, or changing this.
THRESHOLD=85    #The amount of disk space used before printing the warning.
DEBUG=false     #If TRUE when running (-d switch) lists all mount points, even if
                #the threshold has not been met.

while getopts ":dh" opt; do
        case $opt in
                d)
                        DEBUG=true
                        ;;
                h)
                        echo USAGE: $0 \[-d\] \[\/mount\/point\/1 ...\]
                        exit 0
                        ;;
                \?)
                        echo Incorrect syntax
                        ;;
        esac
        shift $((OPTIND - 1))
done

if [ "$#" -gt 0 ]; then
        for var in $@; do
                MOUNTPOINTS="${MOUNTPOINTS} ${var} "
        done
fi

for MOUNT in ${MOUNTPOINTS}; do
  CURRENT=$(df ${MOUNT} | grep / | awk '{ print $5}' | sed 's/%//g')
  if ($DEBUG); then
    printf "%20s\t%s\n" ${MOUNT} ${CURRENT}%
  fi
  if [ "${CURRENT}" -gt "${THRESHOLD}" ] ; then
      echo "Your ${MOUNT} partition\'s remaining space is critically low. Used: ${CURRENT}%"
    fi

done

Solution

Using a string for MOUNTPOINTS means you can't have any mount points with spaces of glob characters in the names. I realize you likely can't use an array if you want real portability but that's a severe limitation that you might want to check for.

I don't know what you can do (without using an array) that will allow directories like that to work but you can check the input arguments for compatibility and disallow any that don't fit those limitations.

As I said on some other question (of yours I believe) that shift in the while loop is incorrect. You are shifting off (an increasing number of arguments) during the getopts processing. You want to do that shift once and after getopts is done.

Try this and see what I mean:

$ cat test-getopts.sh
#!/bin/sh

while getopts ":dh" opt; do
    case $opt in
        d) echo DEBUG=true;;
        h) echo HELP;;
        \?) echo error;;
    esac
    printf 'Shifting %d\n' $((OPTIND - 1))
    shift $((OPTIND - 1))
done
$ /bin/sh go.sh -d -h -d -h -d -h -d -h -d -h -d -h
DEBUG=true
Shifting 1
DEBUG=true
Shifting 2
HELP
Shifting 3
HELP
Shifting 4


There is almost never a reason to pipe grep to awk or sed to awk or awk to sed. awk can almost certainly do everything you need.

So the grep | awk | sed part of $(df ${MOUNT} | grep / | awk '{ print $5}' | sed 's/%//g') can be replaced by a single awk call:

`$(df "${MOUNT}" | awk '/\/{gsub(/%/, ""); print}')`


You'll also notice I quoted "${MOUNT}" there. In general you always want to quote shell variable expansions. In this case it can't actually matter since anything these quotes would protect will already have been expanded/word-split by the for MOUNT in ${MOUNTPOINTS} loop but it is the right thing to do anyway.

Also related to df if you are going to try to field-parse the output you want to use the -P/--portability option. It keeps df (at least from coreutils) from wrapping long lines to get columns to line up (and breaking field counts).

The syntax of if tests in the shell does not require wrapping parentheses. In fact, adding them spawns a sub-shell. So if ($DEBUG); then is running the value of $DEBUG as a command in a sub-shell. If you were to have set DEBUG=debug instead of DEBUG=true you'd have gotten an error from that because debug isn't a valid command. Drop them and use a non-empty or value checking test instead. Either if [ -n "$DEBUG" ]; or if [ "$DEBUG" = DEBUG ];.

You don't need to quote single quotes in a double quoted string so:

echo "Your ${MOUNT} partition\'s remaining space is critically low. Used: ${CURRENT}%"


should be echoing partition\'s out which probably isn't what you want.

for var in $@; do can be simplified to for var; do if you wanted and if you don't then you should use for var in "$@"; do though again given that spaces/glob characters are going to be a problem later this isn't as useful as it might be (though it does let you test for them correctly and avoid them as discussed above).

Also, if you haven't already, you should run your code through shellcheck.

Code Snippets

$ cat test-getopts.sh
#!/bin/sh

while getopts ":dh" opt; do
    case $opt in
        d) echo DEBUG=true;;
        h) echo HELP;;
        \?) echo error;;
    esac
    printf 'Shifting %d\n' $((OPTIND - 1))
    shift $((OPTIND - 1))
done
$ /bin/sh go.sh -d -h -d -h -d -h -d -h -d -h -d -h
DEBUG=true
Shifting 1
DEBUG=true
Shifting 2
HELP
Shifting 3
HELP
Shifting 4
`$(df "${MOUNT}" | awk '/\/{gsub(/%/, ""); print}')`
echo "Your ${MOUNT} partition\'s remaining space is critically low. Used: ${CURRENT}%"

Context

StackExchange Code Review Q#77025, answer score: 5

Revisions (0)

No revisions yet.