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

[Jacob Bachmeyer]

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