Take the 2-minute tour ×
Server Fault is a question and answer site for professional system and network administrators. It's 100% free, no registration required.

I created bash script wich works as Nagios plugin. Here is a part of it:

44  CURDUPLEX=`ethtool $IFACE|grep "Duplex"|cut -d ':' -f 2|xargs`
45  CURSPEED=`ethtool $IFACE|grep "Speed"|cut -d ':' -f 2|xargs|sed 's/.\{4\}$//'`
46  
47  if [ $CURSPEED -eq $SPEED ]; then
48          if [ $CURDUPLEX = $DUPLEX ]; then
49                  echo "OK: Interface $IFACE link at ${CURSPEED}Mb\s and $CURDUPLEX duplex."
50                  exit $OK
51          else
52                  echo "CRITICAL: Interface $IFACE link at ${CURSPEED}Mb\s [OK] and $CURDUPLEX duplex (Expected $DUPLEX)."
53                  exit $CRIT
54          fi
55  elif [ $CURDUPLEX = $DUPLEX ]; then
56          echo "CRITICAL: Interface $IFACE link at ${CURSPEED}Mb\s and $CURDUPLEX duplex [OK]."
57          exit $CRIT
58  else
59          echo "CRITICAL: Interface $IFACE link at ${CURSPEED}Mb\s and $CURDUPLEX duplex (Expected $DUPLEX)."
60          exit $CRIT
61  fi
62  }

When I run it I see the error:

]# ./check_physlink usage
 ./check_physlink: line 48: syntax error near unexpected token `then'
 ./check_physlink: line 48: `        if [ $CURDUPLEX = $DUPLEX ]; then'

I think about for an hour yet, and cannot to understand what is wrong. Thought about no space between left bracket and dollar sign, but no as you see.

Please advise. Thank you.

share|improve this question
3  
= is used for string comparison so you should probably be double quoting your variables as strings (especially to prevent any funkiness in edge cases with whitespace). –  Garrett Jul 27 '12 at 20:19
    
Is the $DUPLEX variable defined when you run ./check_physlink usage? As gman said, you need to better handle the comparison strings inside the brackets, likely by using double quotes. Take a look at mywiki.wooledge.org/BashPitfalls in general, and, say, #4 in particular. –  cjc Jul 27 '12 at 21:07
add comment

2 Answers

up vote 0 down vote accepted

Why the xargs? I had to look it up in the POSIX spec to be reminded that it defaults to echo, but I rarely see it used that way. The Gnu man page doesn't even mention that.

44  CURDUPLEX=`ethtool $IFACE|grep "Duplex" ...`
45  CURSPEED=`ethtool $IFACE|grep "Speed" ...`

Calling ethtool twice gives Nagios performance a bad name.

Try something like:

toolsays=$( ethttool $IFACE )
CURDUPLEX=$( echo "$toolsays" | grep Duplex ... )
CURSPEED=$( echo "$toolsays" | grep Speed ... )

Then, the original script cuts of the last four characters from speed, but later on the code still assumes "Mb/s". Make that assumption explict, so the script will fail when it encounters a host where ethtool does not report the speed in Mb/s.

You can go a step further, and replace the two pipelines functions like:

function duplex_state { typeset toolsaid=$1 s=
    : ${toolsaid:?}
    s=${toolsaid##*Duplex: }
    echo ${s%%$'\n'*}
}
function speed_value { typeset toolsaid=$1 s=
    : ${toolsaid:?}
    s=${toolsaid##*Speed: }
    echo ${s%%Mb/s*}
}

And here's how you call them:

CURDUPLEX=$( duplex_state "$toolsays" )
CURSPEED=$( speed_value "$toolsays" )

Note how the captured output from ethtool still needs to be quoted when not used in an assignment to preserve any linebreaks.

share|improve this answer
add comment

Since you're writing for bash, you should really use [[, not [, for tests. Oh, and USE MORE QUOTES!

Here are some guide and FAQ entries to get you started on improving the script:

Why `` is bad

Using quotes

General practices

share|improve this answer
    
Thanks for tips & tricks. I'll to improve. –  Evgeniy Yablokov Jul 31 '12 at 17:02
1  
With just [ you will need quotes. With [[, they are optional, unless you want to escape wildcards. –  Henk Langeveld Aug 14 '12 at 8:47
    
shopt -s -o nounset is also very helpful, it warns you when a variable is not set e.g. really good at catching spelling type errors. –  davey May 2 '13 at 15:50
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.