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="[email protected],[email protected]"
# 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
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Feb 10, 2015 at 10:59
\$\endgroup\$

2 Answers 2

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="[email protected],[email protected]"
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
janos
113k15 gold badges154 silver badges396 bronze badges
answered Feb 10, 2015 at 14:07
\$\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.

answered Feb 10, 2015 at 14:16
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.