HiveBrain v1.2.0
Get Started
← Back to all entries
patternshellMinor

Shell script to detect router slider status

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptroutersliderstatusshelldetect

Problem

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

Solution

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, ") {
  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:

  • It only reads /sys/kernel/debug/gpio once



  • It's a lot safer



  • It's easier to express more complex logic



  • 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.

Code Snippets

if [ $status != $test ] ; then
                ^---- should use "$test"

elif [ $status == $test ] ; then
               ^---- == not supported in POSIX sh
#!/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";
}

Context

StackExchange Code Review Q#102343, answer score: 4

Revisions (0)

No revisions yet.