Shell script to detect router slider status

Question

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

Answer 1

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:

• http://www.shellcheck.net
• checkbashisms on Debian / Ubuntu systems

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.

• andyADD: 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)
• ErikR: 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: Is there a space after sw1 and sw2 in the output, e.g. gpio-20 (sw1 ) in hi?
• andyADD: there is more than one space, it might be 5-6 spaces.
• ErikR: 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.

Answer 2

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?

Answer 3

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

• andyADD: lol I didn't even think of adding $test to case statement..