3
\$\begingroup\$

Original post: nagios plugin webdav check

According to the good advice I have received I've made some modifications to the code:

#!/bin/bash
#
# check a WebDAV server
# connect using a username and password and upload & delete a test file

#Exit codes
STATE_OK=0
STATE_WARNING=1
STATE_CRITICAL=2
STATE_UNKNOWN=3

FILENAME=test_monitor
FQFN=/tmp/$FILENAME

# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a warning status
[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit $STATE_UNKNOWN; }

HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4
BASEURL=http://$HOSTNAME/$WEBDAVDIR

# create temp file
truncate -s 1M $FQFN
if [ ! -f $FQFN ]; then
    echo "Test file not found!"; exit $STATE_UNKNOWN;
fi

# check if curl is installed
command -v curl >/dev/null 2>&1 || { echo >&2 "Curl is not installed. Script cannot run."; exit $STATE_UNKNOWN; }

function _curl {
curl --fail -u $USERNAME:$PASSWORD "$@"
}

# connect to server and upload file
_curl -s -T $FQFN $BASEURL/ > /dev/null

status=$?

case $status in
    0)
        echo "OK - File uploaded on WebDAV Server: $HOSTNAME"
        # remove test file
        _curl -X DELETE $BASEURL/$FILENAME
        exit $STATE_OK
        ;;
    1)
        echo "CRITICAL - Cannot upload file on $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    6)
        echo "CRITICAL - Could not resolve host $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    26)
        echo "CRITICAL - Cannot open $FQFN for upload"
        exit $STATE_CRITICAL
        ;;
    22)
        echo "CRITICAL - The requested URL returned error"
        exit $STATE_CRITICAL
        ;;
    127)
        echo "CRITICAL - Command not found"
        exit $STATE_CRITICAL
        ;;
    *)
        echo "UNKNOWN - "
        exit $STATE_UNKNOWN
esac

Do you have any more recommendations regarding syntax or script functionality?

I am thinking about the removal of the test file. It is done if the upload is successful, but can it be assumed that if the upload is successful, the delete will be also? In my tests this has always been the case.

Is there a chance for the delete command to fail? And should I modify the script somehow to accommodate for that?

\$\endgroup\$
3
\$\begingroup\$

Thanks for incorporating the various suggestions and posting a new code review. This is looking really excellent. Some nits and answers to your concerns:

  • creating a function for curling is good. "$@" is the right choice as well. But it would be nice if the stuff within squiggly brackets was indented like:

    function _curl {
        curl --fail -u $USERNAME:$PASSWORD "$@"
    }
    
  • I would stick with deleting the temporary file each time so the test is complete each time. One scenario that might happen would be for the disk to fill up, and so no WebDAV writes are possible, but your test succeeds because it has the space of the existing file to work with or maybe there's code so it doesn't rewrite the file for performance reasons. Either way leads to a false negative check which is best avoided.

  • It would be more thorough and nice to check the return value from the delete as well. Doing this with a nested case statement could get unwieldy and certainly duplicates a lot of code. If you can turn the case statement into its own function that could bucket the return values for creates or deletes that might be easier to maintain. There might be other nagios checks where you want to check the output of curl similarly which would be good to keep in mind when designing this curl return evaluator.
  • Aren't you linking to the wrong prior question? nagios plugin webdav check was how I got here. :)
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer, I have updated my code accordingly: pastebin.com/99NL2FDj \$\endgroup\$ – anarchist Feb 8 '16 at 9:51
  • \$\begingroup\$ NIcely done, anarchist. Keep up the good work. \$\endgroup\$ – chicks Feb 8 '16 at 15:30
2
\$\begingroup\$

Instead of this:

function _curl {
curl --fail -u $USERNAME:$PASSWORD "$@"
}

The recommended writing style is this for functions:

_curl() {
    curl --fail -u $USERNAME:$PASSWORD "$@"
}

Instead of cramming multiple commands on one line, it's more readable (and recommended) to split to multiple lines, especially for long lines that don't fit on one screen without scrolling. So instead of this:

[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit $STATE_UNKNOWN; }

This writing style would be easier to read:

[ $# -ne 4 ] && {
    echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"
    exit $STATE_UNKNOWN
}

Even this if statement:

if [ ! -f $FQFN ]; then
    echo "Test file not found!"; exit $STATE_UNKNOWN;
fi

would be better written so you have one command per line:

if [ ! -f $FQFN ]; then
    echo "Test file not found!"
    exit $STATE_UNKNOWN
fi

To redirect both input and output, instead of this:

command -v curl >/dev/null 2>&1

A simpler way to write in Bash:

command -v curl &>/dev/null
\$\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.