1
WBINFO="/usr/bin/wbinfo -t"
TMP="/tmp/winbind"
RESTART="/sbin/service winbind restart"
TXT="failed"


$WBINFO > $TMP
TARGET='cat $TMP |grep $TXT | wc -l'

if [ "$TARGET" -eq "1" ];
then
$RESTART
else
echo good

fi

I get this error:

line 10: [: cat $TMP |grep $TXT | wc -l: integer expression expected

3 Answers 3

3

Single-quoted strings don't expand $FOO into the contents of the variable FOO. Use double-quotes (").

Further, it looks like you're wanting the contents of TARGET to be the output of the cat command. If so, you probably want:

TARGET=$(cat "$TMP" | grep "$TXT" | wc -l)

Even further further, cat file | grep pattern is suboptimal - grep knows how to take files as arguments to parse rather than invoking cat, which is an whole other process to spawn. You probably really want:

if [[ $( grep -c "$TXT" "$TMP" ) -eq 1 ]]; then
0
3
TARGET='cat $TMP |grep $TXT | wc -l'

This assigns the literal string 'cat $TMP |grep $TXT | wc -l' to the variable $TARGET.

It looks like what you want is the output of the command, which requires backticks:

TARGET=`cat $TMP |grep $TXT | wc -l`

or, if you have a reasonably modern shell, the $(...) syntax:

TARGET=$(cat $TMP |grep $TXT | wc -l)

Furthermore, that command can be simplified considerably, from the above to this:

TARGET=$(grep $TXT $TMP | wc -l)

to this:

TARGET=$(grep -c $TXT $TMP)

Finally, the $TARGET variable can be eliminated altogether if you change the if statement from this:

if [ "$TARGET" -eq "1" ];

to this:

if [ $(grep -c "$TXT" "$TMP") = 1 ];

Or you can use [[ ... ]] rather than [ ... ] (it's preferred for bash).

Or, if you only care whether the pattern occurs at all (rather than requiring it to appear exactly once):

if grep -q "$TXT" "$TMP";

For that matter, you can eliminate the $TMP file as well; I'll leave that as an exercise. 8-)}

Consult the documentation for grep to see what the options do. (The -c option tells it to print the number of matches, -q prints nothing but still sets the status to indicate whether the pattern was found).

Note that I also added quotation marks around the variable references, which is good practice if there's any possibility that their values might contain any special characters.

7
  • 1
    Good answer, but I would like to point out that (though not of apparent utility in this case) the question's script explicitly tested for the number of matches to be exactly 1, not to run $RESTART if there were somehow two or more matches in the file (:
    – DopeGhoti
    Commented Feb 12, 2014 at 1:07
  • 1
    Great answer, but I assume you meant -q -- as you mention in the text below -- rather than -s in your last code snippet; also, the , shouldn't be there. Finally, it's also worth mentioning that in bash [[ is preferred over [.
    – mklement0
    Commented Feb 12, 2014 at 2:56
  • @mklement0: Thanks. I'm seriously considering paying more attention to what I write. Commented Feb 12, 2014 at 4:42
  • :) Thanks for updating; the -s is still there, though.
    – mklement0
    Commented Feb 12, 2014 at 4:50
  • 1
    Looks good - thanks for being committed to getting this right.
    – mklement0
    Commented Feb 12, 2014 at 23:59
0

change this line

TARGET='cat $TMP |grep $TXT | wc -l'

to

TARGET=$(cat $TMP |grep $TXT | wc -l)

or

TARGET=`cat $TMP |grep $TXT | wc -l`

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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