----- Original Message -----
From: "Petr Muller" <pmuller(a)redhat.com>
To: beakerlib-devel(a)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
1) prefix messages so the user knows what is emitting them
ack
2) 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.
3) Do not exit the script for the user (see the general comment
above)
(see general answer above) ;)
[...]
will fix other mentioned problems
--
Regards,
Hubert Kario
Quality Engineer, QE BaseOS Security team
http://wiki.brq.redhat.com/hkario
Email: hkario(a)redhat.com
Web:
www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic