4

The shell script I'm trying to use keeps giving this error:

$ ./script.sh: line 2: [: missing `]' 
grep: ]: No such file or directory

The line is part of a section trying to check if a particular process is going to have a file locked:

COUNTER=0
while [ ps aux | grep "[r]elayevent.sh" ] && [ "$COUNTER" -lt 10 ]; do
    sleep 3
    let COUNTER+=1
done

Obviously I've checked that the brackets all pair up correctly - which looks fine to me. Also the common white space around the condition issue doesn't apply.

What am I missing here?

  • 2
    Note for the future. Whenever you get a syntax error, run - don't walk - straight to shellcheck.net and paste in the block of your code that exhibits the issue. – roaima Feb 6 '19 at 8:37
  • You should use flock command to implement locks in scripts. – dnt Feb 6 '19 at 10:03
7

The error is that you should remove first [ because of you want to check the exit status then use command directly.

The Wiki pages of the Shellcheck tool have an explanation for this (issue SC1014):

[ .. ] is not part of shell syntax like if statements. It is not equivalent to parentheses in C-like languages, if (foo) { bar; }, and should not be wrapped around commands to test.

[ is just regular command, like whoami or grep, but with a funny name (see ls -l /bin/[). It's a shorthand for test.

If you want to check the exit status of a certain command, use that command directly.

If you want to check the output of a command, use "$(..)" to get its output, and then use test or [/[[ to do a string comparison:

Also use ps aux | grep -q "[r]elayevent.sh" so that you will get the exit status silently instead of printing anything to stdout.

Or you can use pgrep and direct it's output to /dev/null.

Use second condition first because it will be more efficient for the last case.

So final script will be like:

#!/bin/bash
COUNTER=0

while [ "$COUNTER" -lt 10 ] && ps aux | grep -q "[r]elayevent.sh"   ; do

    sleep 3

    let COUNTER+=1

done

Or

#!/bin/bash
COUNTER=0

while [ "$COUNTER" -lt 10 ] && pgrep  "[r]elayevent.sh" >/dev/null  ; do

    sleep 3

    let COUNTER+=1

done
| improve this answer | |
  • 4
    You probably want to add -q when calling grep, too. Otherwise it prints the matching lines. – Mikel Feb 6 '19 at 4:21
  • @Mikel thanks, it will silently give status instead of printing anything. – Prvt_Yadav Feb 6 '19 at 4:25
  • You should mention where that quote is from. Also, grep -q "[r]elayevent.sh" doesn't make much sense, and ps | grep is usually better replaced with pgrep; but the let COUNTER+=1 is fine in bash, zsh and ksh39 (((..)) is not a bit more portable) -- though the whole script doesn't make much sense ;-)). – mosvy Feb 6 '19 at 4:59
  • 1
    I would swap the tests around to avoid running grep on the last iteration. – Kusalananda Feb 6 '19 at 7:31
  • That is more efficient. – Prvt_Yadav Feb 6 '19 at 7:49
2

You can't have a pipe inside [ ... ]. It's also better to use pgrep than to try to parse the output of ps:

count=0
while [ "$count" -lt 10 ] && pgrep relayevent.sh >/dev/null; then
    sleep 3
    count=$(( count + 1 ))
done

BSD systems could use pgrep -q ... instead of pgrep ... >/dev/null to discard the actual output of pgrep, just as with ordinary grep (we're only interested in the exit status).

Note how we don't put the pgrep command within [ ... ]. That's because we're not interested in its output, only its exit status. With [ ... ] you commonly compare strings or numbers. The [ ... ] will result in an exit status that is zero (true) or non-zero (false), just like the pgrep execution.

However, this does not check for any locking mechanisms, only whether a particular process is running or not.

If you are trying to get only a single instance of a script running, then it's better to do something like this (assuming the EXIT trap is executed whenever the script is terminating orderly):

lockdir=dir.lock

if mkdir "$lockdir"; then
    trap 'rmdir "$lockdir"' EXIT
else
    echo 'Only one instance of this script allowed' >&2
    exit 1
fi

With a number of tries and sleep:

lockdir=dir.lock

count=0
while [ "$count" -lt 10 ]; then
    if mkdir "$lockdir"; then
        trap 'rmdir "$lockdir"' EXIT
        break
    else
        echo 'Locked. Sleeping...' >&2
        sleep 3
    fi

    count=$(( count + 1 ))
done

if [ "$count" -eq 10 ]; then
    echo 'Giving up.' >&2
    exit 1
fi

Related:

| improve this answer | |
  • I also thought of pgrep but didn't thought of /dev/null. – Prvt_Yadav Feb 6 '19 at 8:43
  • @PRY pgrep on BSD systems have a -q option to avoid that particular ugliness. It works just as with grep. – Kusalananda Feb 6 '19 at 8:46
  • @PRY you could use pkill -0 ... instead of pgrep ... >/dev/null – Uncle Billy Feb 6 '19 at 8:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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