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