Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What my script does is:

  1. Append data to a file on a continuous basis.
  2. If an error pattern is observed, then do something.

I need feedback on the way I am grepping for patterns in the continuously appended file. For grep, I am creating a pattern file first and then using it with -f option to grep. Can I do this better? Any other better way I can write my over all logic.

Here's my script

#!/bin/bash
# List of error patterns to look for. Add more patterns to this list
p1='pattern1'
p2='pattern2'
p3='pattern3'
p4='pattern4'

PATTERNFILE=~/patternfile
FILETOCHECK=~/filetocheck

captureFileToCheck()
{
   //appends data into filetocheck &

}
createPatternFile()
{

    for i in `seq 100` # hopefully 100 is all we will need
    do
        if [ ! "x$p{$i}" = "x" ]
        then
            echo $p{$i} >> $PATTERNFILE
        else
            break
        fi
    done
}

createPatternFile
captureFileToCheck

while [ 1 ]
do

    if grep -f $PATTERNFILE $FILETOCHECK
    then
        echo "something starting"

          #doSometing

        echo "something complete"

        break
    fi

done
share|improve this question
    
Any specific issue you're having? Security? Efficiency? Accuracy? Complexity? –  l0b0 Feb 21 '12 at 8:09
add comment

2 Answers 2

up vote 1 down vote accepted

Here is another go at your function

patterns() {
cat <<EOF
pattern1
pattern2
pattern3
pattern4
EOF
}

However, the $patternfile is not a necessity. I would go with just

while :
do    
  grep -f <(patterns) $filetocheck && {
    echo found;
    break
  }
done
share|improve this answer
    
This is a good way to do it. I am concerned about portability! –  abc Apr 21 '12 at 5:36
    
Ignore my comment about portability. Arrays are ok since this is explicitly bash. –  rahul Apr 21 '12 at 15:17
add comment

Use more quotes, as the saying goes.

I find while true is more clear, but the idiomatic while : is also popular.

You might want to use an array for the patterns, then you can simply loop over it (for pattern in "${patterns[@]}") instead of using seq and the if statement.

Uppercase variables are usually used for exports; that is, stuff which is relevant outside your script.

You should avoid cluttering ~; use tmp_dir="$(mktemp -d)" instead.

share|improve this answer
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.