5

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
  • 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. Commented Feb 6, 2019 at 8:37
  • You should use flock command to implement locks in scripts.
    – dnt
    Commented Feb 6, 2019 at 10:03

2 Answers 2

10

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
6
  • 4
    You probably want to add -q when calling grep, too. Otherwise it prints the matching lines.
    – Mikel
    Commented Feb 6, 2019 at 4:21
  • @Mikel thanks, it will silently give status instead of printing anything.
    – Prvt_Yadav
    Commented Feb 6, 2019 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 ;-)).
    – user313992
    Commented Feb 6, 2019 at 4:59
  • 1
    I would swap the tests around to avoid running grep on the last iteration.
    – Kusalananda
    Commented Feb 6, 2019 at 7:31
  • That is more efficient.
    – Prvt_Yadav
    Commented Feb 6, 2019 at 7:49
4

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:

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

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .