Script peer review

Here are some changes I'd recommend, and why I recommend them (the full .diff is attached):

< #!/bin/bash

#!/bin/sh

• I generally avoid using /bin/bash as the invoking shell, to avoid compatibility concerns. These days, it's moot, because pretty much any computer that has /bin/sh has /bin/bash.

< emailwarn=6
< popupwarn=12
< restardays=7

emailWarn=6 popupWarn=12 restartDays=7

• I like consistency.
• "restardays" was misspelled.

/usr/bin/logger -i "$0: Checking uptime."

• The next log entry makes much more sense with this one in place.

< days=uptime | awk '{ print $4 }' | sed 's/,//g'
< num=uptime | awk '{ print $3 }'
< consoleuser=ls -l /dev/console | awk '{ print $3 }'

unit=$(/usr/bin/uptime | /usr/bin/awk '{ print $4 }' | /usr/bin/sed 's/,//g') num=$(/usr/bin/uptime | /usr/bin/awk '{ print $3 }') consoleUser=$(ls -l /dev/console | /usr/bin/awk '{ print $3 }')

• I think "unit" is a more appropriate name than "days" for this variable.
• I find $( ) to be more readable than ` `. They are equivalent, but the big difference is that $( ) can be nested.
• When writing a script that will run as root it's a very good practice to ALWAYS use full paths for commands (except built-in commands). This helps prevent malicious commands from getting escalated privileges.

< if [[ $days != "days" ]]; then

if [ "$unit" != "days" ]; then

• More quotes help keep input sanitized. Remember little Bobby Tables<http://xkcd.com/327/>.
• I've never used [[ ]]; only [ ]. I'm not sure what the difference is, but I know how [ ] works.

< logger -i "$0: Machine is up less than 2 days. No business here. Exiting."

/usr/bin/logger -i "$0: Machine has been up less than 2 days. No business here. Exiting."

• Grammar fix.

< exit 0

• This is unneeded, since control falls off the end of the script outside of the if.

< elif [[ $days = "days" && $num -gt $restardays && $consoleuser = "root" ]]; then

elif (( "$num" > "$restartDays" )) && [ "$consoleUser" == "root" ]; then

• $days check is superfluous; by the time you get here, it's already been checked.
• Arithmetic comparison (( )) is probably what you want when comparing numbers.

< logger -i "$0: No user is logged in and machine is up for $num days. Restarting machine."

/usr/bin/logger -i "$0: No user is logged in and machine has been up for $num $unit. Restarting machine."

• Grammar fix.
• Practically, using "days" here makes no difference. It just feels right to me to use $unit.

subject="Machine Up Too Long"

• If you're going to parameterize the message, you may as well parameterize the subject too.

< message2="important to restart machines regularly to order to run efficiently and to apply updates "

message2="important to restart machines regularly to order to run efficiently and to apply updates"

• Extra space at the end of the line.

< email=dscl "/Active Directory/All Domains/" -read /Users/$CurrentUser | grep EMailAddress | awk '{ print $2 }'

email=$(/usr/bin/dscl "/Active Directory/All Domains/" -read "/Users/$currentUser" EMailAddress | /usr/bin/awk '{ print $2 }')

• The grep is unneeded; just pull the info you want.

< touch /Library/Application Support/JAMF/Receipts/UpTooLong.pkg

/usr/bin/touch "/Library/Application Support/JAMF/Receipts/UpTooLong.pkg"

• I like quoting my paths when they have spaces.

popupTitle="PLEASE RESTART" popupMessage="Your computer has been up for $num $unit. Please restart ASAP."

• Parameterizing messages.

< # Screen Popup to logged in user

# Display popup to logged in user

• I think that's what you meant to say here.

< exit 0

• Exit status is 0 if the last command executed succeeded. If it didn't for some reason, it might be useful to not hide that fact.

You may want to check for other users who might be logged in (try $(/usr/bin/who | /usr/bin/grep console) ). You could loop through them and display notices or send emails to each. And it might not be wise to assume nobody is logged in just because the login window is showing.

These might not matter for you right now, as you might not have Fast User Switching enabled. But one day, you (or someone else) might enable it, and now you have to fix your script (once you've found the cause of the very subtle bug). Better to be prepared, since you can do so.

You may want to use osascript to display the dialog instead of jamfHelper. This way you can include a Restart button (which can execute a restart via Apple Event, to allow unsaved documents and such to be handled appropriately).

Thanks for the review! It's been educational for me (and hopefully other novices) to see another way of doing this especially with the why.

I did a lot of restructuring and changing of the script (like the restart) and did think of the potential multiple user issue. I didn't address it because we don't allow fast user switching or ssh for standard users. You're right though - it's better to address it now than discover it later.

Anyone have other suggestions?

BTW, implementation of this has resulted in ~90% of the machines to an uptime of less than a week. It used to be around 50% or less.

i usually use just awk in place of grep + awk constructs and prefer heredoc style blocks to multiline echo statements. a slightly modified version is here, and you can see the diff from the original.

https://gist.github.com/2369b8ce9ac1c2737bf4

does not restarting often cause significant problems for your users?

Thanks for your version.

Yes, lack of restarts was often an issue. Most of the 100 or so machines are used heavily with the creative suite and iTunes, firefox, etc. I got tired of "fixing" random issues with a restart. Uptimes of months were not uncommon.

I've found myself increasingly trying to use shell scripts and the Casper suite to tackle things like this - so much that I think I'm going to read a book!