Maciej W. Rozycki wrote:
On Wed, 20 May 2020, Jacob Bachmeyer wrote:
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.)
I've tried that, but there is a syntax error in your proposal as the
braces are not balanced.
Oops, yes, that was a typo.
This:
catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}
...was the intended meaning. The outermost braces were intended to
separate a piece of Tcl code from the surrounding text, except one of
them was missing. Oops, mea culpa, on that.
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.)
This is a rare corner case though and I would therefore recommend my
minimal fix to be applied first, and then you can apply your code
modernisation to all `exec' invocations throughout `close_wait_program'.
This way changes will be functionally consistent and won't mix fixes with
clean-ups (or indeed address independent issues at a time).
Even if you are using the previous exec(n) command exactly as is, the
catch(n) script should still be braced, since you are adding the
catch(n) command. Including the exec(n) cleanup also has a benefit of
ensuring that the cleanup does not reintroduce the very bug you are
fixing or a similar bug. When testing is as involved as verifying
DejaGnu currently is, grouping related changes is beneficial because it
allows one verification run to validate related changes.
-- Jacob
