Linux HDD/CPU Temperature Logging Bash Script to run every minute in crontab

Question

I wrote a simple script to log HDD and CPU temps to a log file. If those temperatures reach a threshold, it will shutdown the server. I also made the script make sure the log files don't get over a certain file size. Any ways to improve the code would be greatly appreciated, as I am a noob to programming.

#!/bin/bash
# Check HDD Temps, CPU temp.. shut down server if any exceed acceptable limit
# Using hddtemp to check HDD temps, sensors command to check CPU temp.
date=$(TZ='America/Los_Angeles' date)
HDD1=$(sudo hddtemp /dev/sdb | cut -d":" -f1,2)
HDD2=$(sudo hddtemp /dev/sdc | cut -d":" -f1,2)
T1=$(sudo hddtemp /dev/sdb | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
T2=$(sudo hddtemp /dev/sdc | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
CPU1=$(sensors | grep "Core" | head -1 | cut -d"+" -f2 | cut -d"." -f1)
CPU2=$(sensors | grep "Core" | tail -1 | cut -d"+" -f2 | cut -d"." -f1)
base_file='/home/brandon/logs/temp1.log'
file='/home/brandon/logs/temp'
log='.log'
error='.error'

find_lowest_file_num() {
        local lowest_file_num=$(ls /home/brandon/logs/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | tail -1)

        if [ -z "$lowest_file_num" ]
        then
                touch $base_file
                echo 1
        else
                echo $lowest_file_num
        fi
}

find_highest_file_num() {
        local highest_file_num=$(ls /home/brandon/logs/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | head -1)

        # if highest_file_num is empty, that means there is no temp files, so create one
        if [ -z "$highest_file_num" ]
        then
                touch $base_file
                echo 1
        #else return highest_file_num
        else
               echo $highest_file_num   
        fi
}

highest_file_num=$(find_highest_file_num)
lowest_file_num=$(find_lowest_file_num)

get_curr_highest_file() {
        local h=${file}${highest_file_num}${log}
        echo $h
}

get_curr_lowest_file() {
        local l=${file}${lowest_file_num}${log}
        echo $l
}

curr_file=$(get_curr_highest_file)

echo "HDD1: ${HDD1}  Temp:${T1}C     ${date} " >> $curr_file
echo "HDD2: ${HDD2}  Temp:${T2}C     ${date} " >> $curr_file
echo "CPU Core 1 Temp:${CPU1}C     ${date} " >> $curr_file
echo "CPU Core 2 Temp:${CPU2}C     ${date} " >> $curr_file

#if files get bigger than 1000k, make a new one
if [ $(du -k $curr_file | cut -f1) -gt 1000 ]
then
        let "highest_file_num++"
        touch $(get_curr_highest_file)
fi

if [ $(du -k /home/brandon/logs | cut -f1) -gt 100000 ]
then
        rm -f $(get_curr_lowest_file)
fi

if [ $T1 -gt 50 ] || [ $T2 -gt 50 ] || [ $CPU1 -gt 80 ] || [ $CPU2 -gt 80 ]
then
        echo "HIGH TEMPS DETECTED!!! HDD1 Temp: ${T1}C    HDD2 Temp: ${T2}C     ${date}" >> ${curr_file}${error}
        echo "CPU1: ${CPU1}C, CPU2: ${CPU2}           ${date}" >> ${curr_file}${error}
        echo "Shutting down..." >> ${curr_file}${error}
        sudo shutdown now
fi

Answer 1

Here is my revision. I changed it that instead of hardcoding the user's home directory the script can just get that from env. Also, I would run this directly as a root account chron task rather than configuring sudo to be able to run shutdown and hddtemp. Finally, instead of using if -z to check for a file's existance, I would use test -f or something. Anyway, not bad at all for a 'noob', I'd give yourself more credit! Here is my version:

#!/bin/bash
# Check HDD Temps, CPU temp.. shut down server if any exceed acceptable limit
# Using hddtemp to check HDD temps, sensors command to check CPU temp.
date=$(TZ='America/Los_Angeles' date)
# Consider running as root directly instead of requiring sudo (unless you want \
# to configure a nonroot user to be able to use `hddtemp` and `shutdown`) \ 
# which would work, but since you're running as chron, I would just run \
# it as a root cron job
test $(id -u) == 0 || {echo 'This script must be run as root'; exit 1}
HDD1=$(hddtemp /dev/sdb | cut -d":" -f1,2) 
HDD2=$(hddtemp /dev/sdc | cut -d":" -f1,2)
T1=$(sudo hddtemp /dev/sdb | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
T2=$(sudo hddtemp /dev/sdc | cut -d":" -f3 | cut -d" " -f2 | cut -c1-2)
CPU1=$(sensors | grep "Core" | head -1 | cut -d"+" -f2 | cut -d"." -f1)
CPU2=$(sensors | grep "Core" | tail -1 | cut -d"+" -f2 | cut -d"." -f1)
logdir="/home/$USER/templogs"
base_file="${logdir}/temp1.log"
file="${logdir}/temp"
log='.log'
error='.error'
test -d $logdir||mkdir $logdir 2>/dev/null

find_lowest_file_num() {
        local lowest_file_num=$(ls ${logdir}/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | tail -1)

        if [ -z "$lowest_file_num" ]
        then
                touch $base_file
                echo 1
        else
                echo $lowest_file_num
        fi
}

find_highest_file_num() {
        local highest_file_num=$(ls ${logdir}/ | grep temp | cut -d"p" -f2 | cut -d"." -f1 | sort -rn | head -1)

        # How about using `test` instead of ` if -z`? 

        test -f "$highest_file_num" || { >"$highest_file_num" ; echo 'Created temp file' ;} && echo "highest_file_num is present"

}

highest_file_num=$(find_highest_file_num)
lowest_file_num=$(find_lowest_file_num)

get_curr_highest_file() {
        local h=${file}${highest_file_num}${log}
        echo $h
}

get_curr_lowest_file() {
        local l=${file}${lowest_file_num}${log}
        echo $l
}

curr_file=get_curr_highest_file

echo "HDD1: ${HDD1}  Temp:${T1}C     ${date} " >> $curr_file
echo "HDD2: ${HDD2}  Temp:${T2}C     ${date} " >> $curr_file
echo "CPU Core 1 Temp:${CPU1}C     ${date} " >> $curr_file
echo "CPU Core 2 Temp:${CPU2}C     ${date} " >> $curr_file

#if files get bigger than 1000k, make a new one
if [ $(du -k $curr_file | cut -f1) -gt 1000 ]
then
        let "highest_file_num++"
        touch $(get_curr_highest_file)
fi

if [ $(du -k $logdir | cut -f1) -gt 100000 ]
then
        rm -f $(get_curr_lowest_file)
fi

if [ $T1 -gt 50 ] || [ $T2 -gt 50 ] || [ $CPU1 -gt 80 ] || [ $CPU2 -gt 80 ]
then
        echo "HIGH TEMPS DETECTED!!! HDD1 Temp: ${T1}C    HDD2 Temp: ${T2}C     ${date}" >> ${curr_file}${error}
        echo "CPU1: ${CPU1}C, CPU2: ${CPU2}           ${date}" >> ${curr_file}${error}
        echo "Shutting down..." >> ${curr_file}${error}
        shutdown now # I would run this as root directly
else
    echo "Temps ok: HDD1: ${T1}, HDD2: ${T2}, CPU1: ${CPU1}, CPU2: ${CPU2}"
fi