dnsperftest icon indicating copy to clipboard operation
dnsperftest copied to clipboard

🚨 shellcheck'ed

Open thomasmerz opened this issue 3 years ago • 10 comments

Before:

$ shellcheck dnstest.sh

In dnstest.sh line 8:
NAMESERVERS=`cat /etc/resolv.conf | grep ^nameserver | cut -d " " -f 2 | sed 's/\(.*\)/&#&/'`
            ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
                 ^--------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

Did you mean:
NAMESERVERS=$(cat /etc/resolv.conf | grep ^nameserver | cut -d " " -f 2 | sed 's/\(.*\)/&#&/')


In dnstest.sh line 46:
        ttime=`$dig +tries=1 +time=2 +stats @$pip $d |grep "Query time:" | cut -d : -f 2- | cut -d " " -f 2`
              ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
                                             ^--^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                                  ^-- SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        ttime=$($dig +tries=1 +time=2 +stats @"$pip" "$d" |grep "Query time:" | cut -d : -f 2- | cut -d " " -f 2)


In dnstest.sh line 50:
        elif [ "x$ttime" = "x0" ]; then
               ^-------^ SC2268 (style): Avoid x-prefix in comparisons as it no longer serves a purpose.

Did you mean:
        elif [ "$ttime" = "0" ]; then


In dnstest.sh line 57:
    avg=`bc -lq <<< "scale=2; $ftime/$totaldomains"`
        ^-- SC2006 (style): Use $(...) notation instead of legacy backticks `...`.

Did you mean:
    avg=$(bc -lq <<< "scale=2; $ftime/$totaldomains")

For more information:
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
  https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
  https://www.shellcheck.net/wiki/SC2006 -- Use $(...) notation instead of le...

After no complains anymore 👍🏻

thomasmerz avatar Mar 02 '22 14:03 thomasmerz

@cleanbrowsing , have you seen my PR and can you please review and merge it? Thanks…

thomasmerz avatar Mar 12 '22 22:03 thomasmerz

@cleanbrowsing , I rebased this PR and shellcheck'ed some more style-warnings that came from some other PRs:

SC2181 (style): Check exit code directly with e.g. 'if mycmd;', not indirectly with $? SC2268 (style): Avoid x-prefix in comparisons as it no longer serves a purpose.

thomasmerz avatar Mar 17 '22 21:03 thomasmerz

Testing dnstest.sh after I shellcheck'ed it: image

thomasmerz avatar Mar 17 '22 21:03 thomasmerz

The issue with this syntax is that it breaks on openbsd's default shell (the standard sh shell - openbsd user here). For example, this is what I get when I run with the changes:

./dnstest.sh: syntax error: `< ' unexpected

That's why I kept the "x$1" syntax and the "if [ $?" format to test for the command results. Not sure what can be done to make it compatible with the standard sh shells and remove this format.

cleanbrowsing avatar Mar 17 '22 22:03 cleanbrowsing

Quick way to test:

$ ./test.sh 
./test.sh[5]: [: test: unexpected operator/operand

file:

#!/bin/sh

if [ $1 = "test" ]; then
    echo "cmd1 = test"
fi

cleanbrowsing avatar Mar 17 '22 22:03 cleanbrowsing

If I change to:

$ cat test.sh 
#!/bin/sh

if [ "x$1" = "xtest" ]; then
    echo "cmd1 = test"
fi

It works:

$ ./test.sh 
$ ./test.sh  test
cmd1 = test

Using OpenBSD 6.9

cleanbrowsing avatar Mar 17 '22 22:03 cleanbrowsing

I added a commit to fix your problem with openbsd's default shell. But I wonder, if shebang says "use bash" why openbsd is ignoring this and uses default shell (that is not bash?) 🤔 …

thomasmerz avatar Mar 17 '22 22:03 thomasmerz

Thanks! Checking that now.

cleanbrowsing avatar Mar 17 '22 22:03 cleanbrowsing

Ping @cleanbrowsing …

thomasmerz avatar Aug 12 '22 07:08 thomasmerz

Ping @cleanbrowsing …

thomasmerz avatar Jun 05 '24 14:06 thomasmerz