[PATCH 1/2] new routines for socket based synchronisation

Petr Muller pmuller at redhat.com
Tue Nov 19 16:52:55 UTC 2013


On Tue, 2013-11-19 at 15:49 +0100, Hubert Kario wrote:
> Add new function, rlWaitForSocket, that can wait for a network service
> to start. To be used instead of `sleep' when testing network-centric
> services.

The idea is great, much appreciated. I'm looking forward to the rest.
The decision to put this into a new file is good and the naming is well
descriptive. Thanks.

I would like to have a testcase for it in the testsuite if possible,
though.

Have you checked the compatibility of the shell code with bash code back
to RHEL4? I currently accept RHEL5+ code into upstream, but I would like
to have some knowledge about where this runs. The comments indicate you
have, but just to be sure...

More specific comments inline.

> ---
> is the fatal error handling OK?

No: by directly exiting, you are e.g. preventing any cleanups from
running (and logs from uploading when run on beaker), unfortunately. My
preference would be to just return a RC and let the user handle the
failure e.g. with rlRun. Use journal logs for messages. It's done this
way everywhere BL.

[...]
> +getopt -T || ret=$?
> +if [ ${ret:-0} -ne 4 ]; then
> +    echo "ERROR: Non enhanced getopt version detected" 1>&2
> +    exit 1
> +fi
> +
> +: <<'=cut'
> +=pod
[...]
Documentation is good.

> +rlWaitForSocket(){
> +
> +    local timeout=120
> +    local proc_pid=1
> +    local socket=""
> +    local child_pid=0
> +
> +    # that is the GNU extended getopt syntax!
> +    TEMP=$(getopt -o t:p: -n 'rlWaitForSocket' -- "$@")
local, please

> +    if [[ $? != 0 ]] ; then echo "Terminating..." >&2 ; exit 1 ; fi
1) prefix messages so the user knows what is emitting them
2) Be more specific with failure messages: what happened? why premature
exit?
3) Do not exit the script for the user (see the general comment above)

> +    eval set -- "$TEMP"
> +
> +    while true ; do
> +        case "$1" in
> +            -t) timeout="$2"; shift 2
> +                ;;
> +            -p) proc_pid="$2"; shift 2
> +                ;;
> +            --) shift 1
> +                break
> +                ;;
> +            *) echo "Internal error!" >&2; exit 1
See 1),2),3) above

> +                ;;
> +        esac
> +    done
> +    socket="$1"
> +    # the case statement is a portable way to check if variable contains only
> +    # digits (regexps are not available in old, RHEL3-era, bash)
> +    case "$timeout" in
> +        ''|*[!0-9]*) echo "rlWaitForSocket: Invalid timeout provided" 1>&2; exit 1;;
See 2),3) above

> +    esac
> +    case "$proc_pid" in
> +        ''|*[!0-9]*) echo "rlWaitForSocket: Invalid PID provided" 1>&2; exit 1;;
See 2),3) above

> +    esac
> +    case "$socket" in
> +        ''|*[!0-9]*)
> +            #socket_type="network"
> +            grep_opt="\:$socket[[:space:]]"
local, please

> +            ;;
> +        "") echo "rlWaitForSocket: Empty socket specified" 1>&2
See 2),3) above
> +            exit 1
> +            ;;
> +        *)
> +            #socket_type="unix"
> +            grep_opt="$socket"
> +            ;;
> +    esac
> +    rlLog "Waiting max ${timeout}s for socket \`$socket' to start listening"
Prefix message

> +
> +    ( while true ; do
> +        netstat -nl | grep -E "$grep_opt" >/dev/null
> +        if [[ $? -eq 0 ]]; then
> +            exit 0;
> +        else
> +            if [[ ! -e "/proc/$proc_pid" ]]; then
> +                exit 1;
> +            fi
> +            sleep 1
> +        fi
> +    done ) &
> +    netstat_pid=$!
local

> +    ( sleep $timeout && kill -HUP $netstat_pid ) 2>/dev/null &
> +    watcher=$!
local

> +
> +    wait $netstat_pid
> +    ret=$?
local

> +    if [[ $ret -eq 0 ]]; then
> +        kill -s SIGKILL $watcher 2>/dev/null
> +        rlLog "Socket opened!"
Prefix

> +    else
> +        if [[ $ret -eq 1 ]]; then
> +            kill -s SIGKILL $watcher 2>/dev/null
> +            rlLogWarning "PID terminated!"
Prefix

> +        else
> +            rlLogWarning "Timeout elapsed"
Prefix

> +        fi
> +    fi
> +}
[...]

PM



More information about the beakerlib-devel mailing list