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.

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