4
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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) \$\endgroup\$ – andyADD Aug 31 '15 at 5:53
  • \$\begingroup\$ 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) \$\endgroup\$ – ErikR Aug 31 '15 at 6:05
  • \$\begingroup\$ Is there a space after sw1 and sw2 in the output, e.g. gpio-20 (sw1 ) in hi? \$\endgroup\$ – ErikR Aug 31 '15 at 6:07
  • \$\begingroup\$ there is more than one space, it might be 5-6 spaces. \$\endgroup\$ – andyADD Aug 31 '15 at 6:18
  • \$\begingroup\$ 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. \$\endgroup\$ – ErikR Aug 31 '15 at 6:37
2
\$\begingroup\$

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?

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

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
\$\endgroup\$
  • \$\begingroup\$ lol I didn't even think of adding $test to case statement.. \$\endgroup\$ – andyADD Aug 31 '15 at 20:18

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.