4
\$\begingroup\$

I use Ubuntu (Trusty and Xenial) and usually put my computer in sleep mode rather than power it down. A common problem is that upon wake the network is down so one has to restart the network-manager service. E.g.

sudo service network-manager restart

I am tired of doing this so I have hacked the following together:

#!/bin/sh

# should live in /lib/systemd/system-sleep/...
# sudo mv restart-network-on-wake.sh /lib/systemd/system-sleep/
# sudo chmod a+x /lib/systemd/system-sleep/restart-network-on-wake.sh

case $1/$2 in
    pre/*)
        echo "Going to $2..."
        exit 0
        ;;
    post/*)
        echo "Waking up from $2..."
        if ! ping -q -c 1 -W 1 8.8.8.8 > /dev/null; then
            echo "IPv4 is down, restarting network-manager"
            service network-manager restart
        fi
        ;;
esac

I am a novice at shell scripting I guess and don't have a strong understanding of Linux internals. Do you have any thoughts?

\$\endgroup\$
2
\$\begingroup\$

Welcome to Code Review. Thanks for asking for a review. Overall I think you did a great job, even for an experienced scripter.

good things

  • consistent indentation
  • helpful comments about installation
  • you didn't forget the #! line
  • using if to check for return values without creating extra variables
  • I'm a fan of bash and prefer it in most cases, but specifying an older shell will help this be portable to environment without bash like embedded systems.

suggestions

  • include a comment explaining what the purpose of the script is. The file name is pretty descriptive, but someone might rename it accidentally. So including the filename in the comment would not be too redundant.
  • run shell code through shellcheck. Lucky for you it didn't find any issues with this code
  • it is a good practice to include a *) in your case to catch the unexpected.
  • I like for there to be a space before the ) in the case matches and a blank line after the ;;. The space before the ) makes it clearer what is part of the pattern and what isn't. case statements can get longish so having the extra space after the ;; may annoy you if you have a small screen, but I find it makes it easier to follow the flow when it is more separated.

main gripe

Why did you include $2 in your case? Since you didn't cross a function call boundary there's nothing in the case statement that will prevent access from $2 inside. I've integrated these suggestions into a rewrite I haven't tested. But it should work and seems cleaner to me:

#!/bin/sh

# restart-network-on-wake.sh - systemd/Ubuntu helper

# INSTALLATION
#
# The script should live in /lib/systemd/system-sleep/...
#   sudo mv restart-network-on-wake.sh /lib/systemd/system-sleep/
#   sudo chmod a+x /lib/systemd/system-sleep/restart-network-on-wake.sh

case $1 in
    pre )
        echo "Going to $2..."
        exit 0
        ;;

    post )
        echo "Waking up from $2..."
        if ! ping -q -c 1 -W 1 8.8.8.8 > /dev/null; then
            echo "IPv4 is down, restarting network-manager"
            service network-manager restart
        fi
        ;;

    * )
        echo "ERROR: got $1 which is not expected"
        exit 1
        ;;
esac
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for your detailed review, I completely agree with all your points. RE: your main gripe, I hacked the script together in part from this answer on askubuntu and I was thinking case $1/$2 in was doing some sort of erlang-like pattern matching/string unpacking but I realise now it is more like a switch statement and that the $1 and $2 are args passed into the script scope by systemd :) \$\endgroup\$ – Pocketsand Dec 6 '17 at 19:48
  • 1
    \$\begingroup\$ Gladly. :) $1, $2, etc. could be arguments in a shell function, but since you aren't doing that they are just the arguments to the whole script. \$\endgroup\$ – chicks Dec 6 '17 at 19:54

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.