Web Hosting Talk







View Full Version : Critique Bash Script


Mark Muyskens
11-18-2009, 02:44 AM
Exactly what the subject says, I'd like as many suggestions as possible. :agree:

Thanks!

#!/bin/bash

# Mark's Health Check
# Mark Muyskens
# mmuyskens@gmail.com
# Released under whatever the hell license you want

DMOUNT="/dev/sda1"
LDISKSPACEGB="10"
LMEMMB="100"
SUBJECT="Health Check for Server `hostname -f`"
EMAIL="mmuyskens@gmail.com"
EMAILMESSAGE="/tmp/emailmessage.txt"
ALERT="/tmp/alert.txt"
STATS="/tmp/stats.txt"

mstotal=$(free -tom | grep "Total:" | awk '{print $2}')
msused=$(free -tom | grep "Total:" | awk '{print $3}')
msfree=$(free -tom | grep "Total:" | awk '{print $4}')
mtotal=$(free -om | grep "Mem:" | awk '{print $2}')
mused=$(free -om | grep "Mem:" | awk '{print $3}')
mfree=$(free -om | grep "Mem:" | awk '{print $4}')
uptime=$(uptime | awk '{print $3}')
load=$(uptime | awk '{print $11}' | sed 's/,//')
tdisk=$(df -h | grep "$DMOUNT" | awk '{print $2}')
udisk=$(df -h | grep "$DMOUNT" | awk '{print $3}')
adisk=$(df -h | grep "$DMOUNT" | awk '{print $4}')
adisksed=$(df -h | grep "$DMOUNT" | awk '{print $4}' |sed 's/G//')
pdisk=$(df -h | grep "$DMOUNT" | awk '{print $5}')

echo "Total memory (including swap): $mstotal MB" >$STATS
echo "Used memory (includeing swap): $msused MB" >>$STATS
echo "Free memory (includeing swap): $msfree MB" >>$STATS
echo "Total memory (excluding swap): $mtotal MB" >>$STATS
echo "Used memory (excluding swap): $mused MB" >>$STATS
echo "Free memory (excluding swap): $mfree MB" >>$STATS
echo "Server Uptime: $uptime Days" >>$STATS
echo "Server Load: $load" >>$STATS
echo "Total Disk: $tdisk" >>$STATS
echo "Used Disk: $udisk" >>$STATS
echo "Avaliable Disk: $adisk" >>$STATS
echo "Percentage Disk Used: $pdisk" >>$STATS

if [ "$adisksed" -lt "$LDISKSPACEGB" ] || [ "$adisksed" -eq "$LDISKSPACEGB" ]
then
echo -e "Please check server `hostname -f` \nCurrent available disk quota is: $adisk \n" > $ALERT
cat $ALERT $STATS > $EMAILMESSAGE
/bin/mail -s "$SUBJECT" "$EMAIL" < $EMAILMESSAGE
fi

if [ "$mfree" -lt "$LMEMMB" ] || [ "$mfree" -eq "$LMEMMB" ]
then
echo -e "Please check server `hostname -f` \nCurrent available memory is: $mfree MB \n" > $ALERT
cat $ALERT $STATS > $EMAILMESSAGE
/bin/mail -s "$SUBJECT" "$EMAIL" < $EMAILMESSAGE
fi

rm -rf $EMAILMESSAGE
rm -rf $STATS
rm -rf $ALERT

e-Sensibility
11-18-2009, 03:00 AM
Your script looks very useful, but I have a few critiques:

Saving the data that you parse to disk is tremendously expensive compared to using data-structures that are stored in memory (i.e. arrays, hashes, etc . . . or the bash equivalents, I don't have much practical bash experience); using these in-memory data structures will also help you to better model the data you're wanting to manipulate, which will make your program more maintainable and extensible.

In keeping with the theme of my first suggestion, you should rely more on iterative control flow statements like for and while loops (the former being an abstraction of the latter in many languages), and you should make your program less linear. For instance, testing the "free -tom" command against multiple conditions within one loop could vastly improve your program's performance.

Lastly, it's hard to not feel like bash isn't the best choice for this use-case. I would tend to favor a language like Perl for parsing text.

This is a neat script, and looks very useful. Please don't take my criticism the wrong way; after all, you did ask for it :)

dexxtreme
11-18-2009, 04:49 AM
Yeah, a language that has data structures that are easier to manage (e.g., Perl) would be good to use in this case. Instead of running "free" 6 times, "uptime" twice, and "df" 5 times, you can run them each once and store the output in an array.

Or, you can be brave and re-write the script using "awk" to parse the output from those commands.

nwmcsween
11-18-2009, 06:55 AM
1. Use /proc/meminfo
2. Use parameter expansion
3. Use if [[]]; then
4. /bin/mail may not exist on some installs
5. Store data in arrays/variables no need to write it all to files.

Mark Muyskens
11-18-2009, 03:54 PM
Thanks, I appreciate all the feedback.

foobic
11-18-2009, 06:39 PM
Without repeating all the above, a "set -eu" can often be useful on bash scripts (e = stop on errors, u = using uninitialised variables will trigger errors), and
# Released under whatever the hell license you want
There's an existing public license (http://sam.zoy.org/wtfpl/) for this. ;)

cselzer
11-18-2009, 08:04 PM
There's an existing public license (http://sam.zoy.org/wtfpl/) for this. ;)

That is awesome.

fastdeploy
11-18-2009, 08:06 PM
I'd like as many suggestions as possible.
Though there are some improvements that could be made to the script - mainly stylistic, so more a matter of opinion than any specific programmatic shortcomings - my first thought is why are you reinventing the wheel when you could use Nagios/Zenoss/etc to far more robustly perform the same kinds of monitoring?

Or is this just an exercise to see if you can do this?

Mark Muyskens
11-19-2009, 12:16 AM
Or is this just an exercise to see if you can do this?

Just the outcome of a slow work day :agree:

PCS-Chris
11-19-2009, 02:06 PM
Looks quite good, but it's a bad habit to use "rm -rf" to remove single files.