What's wrong with getopts or grep here

Question

    #!/bin/bash

    #return codes:
    STATE_OK=0
    STATE_WARNING=1
    STATE_CRITICAL=2
    STATE_UNKNOWN=3

    if [ $# -lt 4 ]
    then
       echo "UNKNOWN: Please, Pass Enough Parameters"
       exit $STATE_UNKNOWN
    fi

    while getopts ":w:c:f:p" opt
    do
     case ${opt} in
       w )
         WARN=${OPTARG}
         ;;
       c )
         CRIT=${OPTARG}
         ;;
       f )
         FILE=${OPTARG}
         ;;
       p )
         PATTERN=${OPTARG}
         ;;
     esac
    done

    COUNT=$(tail -50 ${FILE} | grep -c '${PATTERN}')


    if [ $? -eq 0 ]
    then
            if [ ${COUNT} -gt ${CRIT} ]
            then
                            echo "CRITICAL: The FAIL count is now ${COUNT}"
                            echo $STATE_CRITICAL

            elif [ ${COUNT} -le ${CRIT} -a ${COUNT} -ge ${WARN} ]
                    then
                            echo "WARNING: The FAIL count is now ${COUNT}"
                            echo $STATE_WARNING
            else
                            echo "OK: The FAIL count is now ${COUNT}"
                            echo $STATE_OK
            fi

    else

            echo "CRITICAL: Error while getting the data"
            echo $STATE_CRITICAL
    fi

grep -c '${PATTERN}' should be grep -ce "$PATTERN". You forgot to quote many of your variables. — Stéphane Chazelas, Jan 27 '17 at 15:31
Please, make your title more concise and try to explain your problem in the body of your question. Also this is not a code review forum, so consider copying only the part of your script that you suspect is at fault (the line with grep here seems like a reasonable assumption). — Valentin B., Jan 27 '17 at 15:35

score 1 · Accepted Answer · edited Apr 13 '17 at 12:37

The argument to grep is given as a single-quoted string. This means that the shell variable PATTERN will not have its value expanded within the string. Instead use "$PATTERN".

Also, as Stéphane Chazelas points out in comments to the question itself, you should double-quote all variable expansions. See "Security implications of forgetting to quote a variable in bash/POSIX shells"

You're also inconsistent in your use of the STATE_ variables. In one situation, you use $STATE_UNKNOWN with exit while in the other cases you echo their values to standard output.

Also note that diagnostic messages (errors or warnings etc. that are not part of the output of the script if it's running as it should) should ideally be outputted onto the standard error stream. To do that use the redirection >&2:

printf 'CRITICAL: The FAIL count is now %d\n' "$COUNT" >&2

Another thing, which is just a style thing, is that bash supports syntax like

if (( COUNT <= CRIT )) && (( COUNT > WARN )); then ...; fi

(in this case the test against CRIT is unnecessary, since it's tested in the previous if statement). This arguably makes it easier to type and read the arithmetic comparisons.

What's wrong with getopts or grep here

1 Answers1