4
\$\begingroup\$

I've made a simple bash script to check if a web server responds, and to send emails to a list of addresses if the website is down. Any suggestions as to how to improve it/ edge cases that I missed/ better styling/ etc? I'm not very familiar with idiomatic bash.

#!/bin/sh

output=$(wget http://lon3213:8111 2>&1)
pattern="connected"
path="/remote/users/my_user/projects/cmb_pinger/"
# File where I check and write the status of the server
# so I don't end up sending more e-mails for the same
# downtime. It writes 0 if no email has been sent yet
# and 1 if the email has been already sent.
log="check"
emails="email1@example.com,email2@example.com"

# Create file if it doesn't exist
if ! [[ -f "$path$log" ]]
then
    echo "0" > "$path$log"
fi

# Website cannot be reached
if [[ ! "$output" =~ "$pattern" ]]
then
    # Emails have not yet been sent
    if [[ $(cat "$path$log") == "0" ]]; then
        echo "$output" | mail -s "Dashboard is down" $emails
        echo "1" >| "$path$log"
    fi
else
    echo "0" >| "$path$log"
fi
\$\endgroup\$
4
\$\begingroup\$

I am impressed with the consistency of your code. You have obviously put effort in to making sure it is well presented, etc. It makes it easy to read.

The first thing I notice is the she-bang line, you have #!/bin/sh this should be #!/bin/bash... right? The script uses Bash specific features that don't exist in classic /bin/sh, such as [[. Although it may appear to work with #!/bin/sh in your system, chances are that your /bin/sh is symlinked to /bin/bash (typically in Linux). This is not something you can rely on (it will break spectacularly in Solaris, for example), so when using Bash-specific features, then you should use #!/bin/bash in the she-bang.

Then, there are some shortcuts you are missing though, and these are standard processes available on most Unix commands.... wget has an exit code! Use it.

wget saves the web content to a file in the local directory. It will not override files, so you may end up with many copies of the same content. I recommend redirecting the URL contents to the stdout by using -O - argument for wget.

Error handling is a critical part of any script. It's not always easy in bash, but it can be done.

You have used the redirection >| in a few places. This caught me by surprise. I admit I looked it up. In my experience the noclobber setting is seldom set, but that may just be for me. That's just an observation. Your site may be set up differently.

Finally, bash has function support, which allows you simplify logic substantially

Consider the following:

#!/bin/bash

url="http://lon3213:8111"
path="/tmp/cmb_pinger/"

# File where I check and write the status of the server
# so I don't end up sending more e-mails for the same
# downtime. It writes 0 if no email has been sent yet
# and 1 if the email has been already sent.

log="check"
emails="email1@example.com,email2@example.com"

error() {
    local message="$@"
    >&2 echo $message
    exit 1
}

setFlag() {
    local flag=$1
    echo $flag >| "$path$log" || error "Unable to set flag to $flag"
}

# Create file if it doesn't exist
[[ -d "$path" ]] || mkdir -p "$path" || error "Unable to create directory $path"
[[ -f "$path$log" ]] || setFlag 0

# Website cannot be reached
output=$(wget -O - $url 2>&1)
success=$?

if [[ $success -eq 0 ]]
then
    setFlag $success
else
    # Emails have not yet been sent
    if [[ $(cat "$path$log") == "0" ]]; then
        echo $output | mail -s "Dashboard is down" $emails || error "Unable to send mail to $emails"
        setFlag $success
    fi
fi
\$\endgroup\$
3
\$\begingroup\$

It's good to quote things to avoid unintended glob expansions or paths with spaces, but when you are typing literal strings that don't contain anything special, then it's a bit overkill. I would drop the double-quotes in all these examples:

pattern="connected"                                          
path="/remote/users/my_user/projects/cmb_pinger/"       
log="check"
echo "0"

To give a counter-example too, this on the other hand is good as it is, the right thing to do:

if ! [[ -f "$path$log" ]]                                    

One tip though, in my experience it's often more ergonomic to not include trailing slashes in path names, and put the / later when concatenating. So like this:

path=/remote/users/my_user/projects/cmb_pinger
log=check

if ! [[ -f "$path/$log" ]]                                    
then                                                         
# ...

The reason I like this better is because the directory boundary between $path and $log is now clear, and I don't need to worry if $path had a trailing slash or not, the script will work either way. It's a minor thing though, I've tried both ways many times, and this way has proven to be more practical for me.

Finally, the biggest issue with the script, is I think the storing and checking of 0 and 1 values in the log file. It's awkward to cat a file to compare its content to a simple value, and I think you can replace this logic by a check on whether the file exists or not, like this:

markerfile="$path/$log"

# Website cannot be reached                                  
if [[ ! "$output" =~ "$pattern" ]]                           
then                                                         
    # Emails have not yet been sent                          
    if [[ ! -f "$markerfile" ]]; then                 
        echo "$output" | mail -s "Dashboard is down" $emails 
        touch "$markerfile"                              
    fi                                                       
else                                                         
    rm -f "$markerfile"
fi  

I also replaced "$path$log" used in multiple places with "$markerfile", which eliminates some duplicated logic.

\$\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.