Re: [PATCH 1/2] remote: Use `catch' in killing pending force-kills

[Jacob Bachmeyer]

Maciej W. Rozycki wrote:
> ---
>  lib/remote.exp |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> dejagnu-remote-close-wait-kill-catch.diff
> Index: dejagnu/lib/remote.exp
> ===================================================================
> --- dejagnu.orig/lib/remote.exp
> +++ dejagnu/lib/remote.exp
> @@ -113,7 +113,10 @@ proc close_wait_program { program_id pid
>         # We reaped the process, so cancel the pending force-kills, as
>         # otherwise if the PID is reused for some other unrelated
>         # process, we'd kill the wrong process.
> -       exec sh -c "exec > /dev/null 2>&1 && kill -9 $exec_pid"
> +       #
> +       # Use `catch' in case the force-kills have completed, so as not
> +       # to cause TCL to choke if `kill' returns a failure.
> +       catch "exec sh -c \"exec > /dev/null 2>&1 && kill -9 $exec_pid\""
>      }
>  
>      return $res
>   

That line is starting to look needlessly complicated.  Does {catch {exec 
sh -c "kill -9 $exec_pid" >& /dev/null} also work?  (I am using the 
outermost braces to denote quoting a piece of Tcl code inline in running 
text.)  The "catch" command recursively invokes the interpreter in the 
same context, so there is no need for variables to be substituted before 
invoking catch(n).  As a matter of modern Tcl (8.0+) style, the script 
argument to catch(n) should be in braces.  If I understand correctly, a 
braced script argument to catch(n) will be byte-compiled when the 
surrounding procedure is loaded, while other scripts must be parsed at 
runtime.  When fixing bugs that only bite under heavy load, efficiency 
should be a consideration.  (I have spent many hours reading the Tcl 
reference manuals; hopefully others can also benefit from my experience 
here.)

Modern Tcl exec(n) also allows I/O redirection, so the only reason to 
invoke a shell at all is to support systems where kill(1) is only 
available as a shell builtin.  This does not include GNU/Linux (where 
/bin/kill is provided by util-linux) and probably should not affect GNU 
at all.  (If it does, either complain until "util-hurd" is released or 
get bash to invoke a builtin if run under the name thereof.)  If we can 
rely on kill(1) being available as an executable on the PATH, it can be 
simplified further to {catch {exec kill -9 $exec_pid >& /dev/null}}.  
(Outermost braces again denote quoting Tcl code inline.)

Most of the complexity in this code is traceable back to the "Initial 
revision" and there are comments about using the shell instead of Expect 
to work around I/O bugs in "older CYGWIN" (added in commit 
4306dace39d6628ca4105c672b1497c3c75ee745, but acknowledged as a "late" 
merge of a change dated 2001-11-12).  On one hand, we should be able to 
move on from compatibility concerns with systems that ancient, but on 
the other hand, the unit tests for this module (in 
testsuite/runtest.libs/remote.test) are currently (still) a stub that 
does nothing, so caution is still the watchword.


-- Jacob