# Shell script to detect router slider status

I have an openwrt router (TP-Link MR3040) and on boot I have it check the slider (AP, WISP, 3G/4G mode). The goal was to have it execute the current "slider status" vs the old one, or do nothing if it's the same (before reboot). I just want a critique on making it more elegant.

#!/bin/sh

test=$(cat /root/logs/sliderstatus) status="" if grep -qe "sw1.*in hi" /sys/kernel/debug/gpio ; then if grep -qe "sw2.*in hi" /sys/kernel/debug/gpio ; then # AP logger "Configure AP" status="AP" else # WISP logger "Configure WISP" status="CLIENT" fi else # 3G logger "Configure 3G" status="CUSTOM" fi if [$status != $test ] ; then case$status in

CLIENT) echo "status is client, executing now"
#sh /root/scripts/shell/CLIENT_MODE.sh &
;;

AP) echo "status is AP, executing now"
#sh /root/scripts/shell/AP_MODE.sh &
;;

CUSTOM) echo  "status is CUSTOM, executing now"
;;

*) echo "ERROR status does not match ERROR"
;;

esac

elif [ $status ==$test ] ; then

echo " Status: $status and Slider$test are the same"

else
echo "You shouldn't have seen this"

fi


First of all, there are so many pitfalls associated with /bin/sh programming that I prefer to write all except the most trivial scripts in a language like perl, python or even awk. I realize that availability is a concern, but all of those languages are pretty standard now.

If you write the script in a better scripting language you can get rid of the duplicate call to grep which is one thing that I presume bothers you about the code.

If you must write in /bin/sh, then run your code through one of the following static analyzers to help you find potential coding problems:

For instance, ShellCheck found these issues:

if [ $status !=$test ] ; then
^---- should use "$test" elif [$status == $test ] ; then ^---- == not supported in POSIX sh  In perl your script would look like this: #!/usr/bin/env perl use strict; use warnings; use File::Slurp; my$test = read_file( '/root/logs/sliderstatus' );
chomp $test; my ($found1, $found2); open(my$fh, "</sys/kernel/debug/gpio");
while (<$fh>) { if (m/$$sw1\s*$$\s+in\s+hi/) {$found1 = 1 }
if (m/$$sw2\s*$$\s+in\s+hi/) { $found2 = 1 } } close($fh);

my ($status,$script);
if ($found1 &&$found2) {
$status = "AP";$script = "/root/scripts/shell/AP_MODE.sh";
} elsif ($found1) {$status = "CLIENT";
$script = "/root/scripts/shell/CLIENT_MODE.sh"; } else {$status = "CUSTOM";
# don't set $script } if ($status ne $test) { print "Status is$status, executing now.\n";
if ($script) { system("$script &");
}
} else {
print "Status $status and slider$test are the same.\n";
}


It's still basically the same code, but has these advantages:

1. It only reads /sys/kernel/debug/gpio once
2. It's a lot safer
3. It's easier to express more complex logic
4. You have more versatile data structures available to you - e.g. real arrays and hash maps.

Finally, I have a question about your patterns for sw1 and sw2. What does the output of /sys/kernel/debug/gpio look like? Presumably the inputs are number something like sw1, sw2, sw3, etc. Could there be a sw10? There should be a delimiter in the output you can use to make sure you are matching the full switch number.

• the output are null (nothing) since my router is in client mode, but if it was in AP mode, it would look like this. gpio-19 (sw1 ) in hi same with the second if statement gpio-20 (sw2 ) in hi. I may not be stuck with shell, I mean this script is suppose to run during boot (in init.d directory) as long as it has the (#!/usr/bin/env perl which i forgot the name bang or something line? idk) – andyADD Aug 31 '15 at 5:53
• You can always day reference the perl interpreter, e.g. #!/bin/perl or wherever perl is located on your system. Read mode about how /usr/bin/env is used here: (link) – ErikR Aug 31 '15 at 6:05
• Is there a space after sw1 and sw2 in the output, e.g. gpio-20 (sw1 ) in hi? – ErikR Aug 31 '15 at 6:07
• there is more than one space, it might be 5-6 spaces. – andyADD Aug 31 '15 at 6:18
• Updated the script with a more robust regex to handle the gpio output. My point is not that you should use this perl script, but that learning a scripting language like perl or python will pay off over the long run. – ErikR Aug 31 '15 at 6:37

## Format:

• One third of your code consists of blank lines almost randomly spread in the code. That does not improve readibility. You should remove them all.

• A size of 8 for tab is rather large. This also makes the code harder to read. Many people use 4, I prefer 2.

• The identation of the case statement is wrong.

• I prefer the identation

if
BLOCK
else
BLOCK
fi


to

if
BLOCK
else
BLOCK
fi


The latter seems to say that ther are three relevant parts.

## Unix:

• Error messages should always be written to stderr, like

echo 'this is a error' >&2

• There is no three valued logic in Unix so the "You shouldn't have seen this" else-clause does not make sense. Use if/else instead of if/elif/else.

• In this script you start another script in parallel (this is indicaed by the comment). Does the calling process/user expect that or does it expect that everything is finihed when the script has finished?

In addition to the other excellent reviews, a minor improvement is to blend the last if-else into the case, like this, with improved formatting:

case $status in$test)
echo "Status: $status and Slider$test are the same"
;;
CLIENT)
echo "status is client, executing now"
#sh /root/scripts/shell/CLIENT_MODE.sh &
;;
AP)
echo "status is AP, executing now"
#sh /root/scripts/shell/AP_MODE.sh &
;;
CUSTOM)
echo "status is CUSTOM, executing now"
;;
*)
echo "ERROR status does not match ERROR"
;;
esac

• lol I didn't even think of adding \$test to case statement.. – andyADD Aug 31 '15 at 20:18