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

[Maciej W. Rozycki]

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.  From your note in the parentheses I infer you 
meant:

        {catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}}

but that results in:

ERROR: (DejaGnu) proc "{catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}}" 
does not exist.
The error code is TCL LOOKUP COMMAND {catch {exec sh -c "kill -9 $exec_pid" >& 
/dev/null}}
The info on the error is:
invalid command name "catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}"
    while executing
"::tcl_unknown {catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}}"
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 ::tcl_unknown $args"

(which does not quite surprise me as the line is then interpreted as a 
single word).  This:

        catch {exec sh -c "kill -9 $exec_pid" >& /dev/null}

does appear to work, including the redirection.

>  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).

 FWIW,

  Maciej