----- Original Message -----
From: "Petr Muller" pmuller@redhat.com To: beakerlib-devel@lists.fedorahosted.org Sent: Tuesday, 19 November, 2013 5:52:55 PM Subject: Re: [PATCH 1/2] new routines for socket based synchronisation
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.
sure, first wanted to know if the implementation is ok
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...
I did write it with limitations of tools in RHEL 3 in mind, haven't actually tested the whole script on it though, just code snippets and tool options.
Will do that before final version.
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.
I'm assuming that by "RC" you mean "return code". I'll change it to do rlLogFatal and "return big-n" on unrecoverable errors then.
[...]
+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
- prefix messages so the user knows what is emitting them
ack
- Be more specific with failure messages: what happened? why premature
exit?
The detailed error messages are printed by getopt itself. If getopt returns non zero it means that the script is trying to use unsupported option so the user script is faulty.
- Do not exit the script for the user (see the general comment above)
(see general answer above) ;)
[...]
will fix other mentioned problems