[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