cleanbrowsing / dnsperftest

DNS Performance test
Other
2.33k stars 279 forks source link

🚨 shellcheck'ed #68

Open thomasmerz opened 2 years ago

thomasmerz commented 2 years ago

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 commented 2 years ago

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

thomasmerz commented 2 years ago

@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 commented 2 years ago

Testing dnstest.sh after I shellcheck'ed it:

image
cleanbrowsing commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

thomasmerz commented 2 years ago

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?) 🤔 …

cleanbrowsing commented 2 years ago

Thanks! Checking that now.

thomasmerz commented 2 years ago

Ping @cleanbrowsing …

thomasmerz commented 5 months ago

Ping @cleanbrowsing …