Re: [PATCH 1/4] ssh.exp: implement ssh_spawn

On 3/24/26 16:20, Joseph Myers wrote:
> This allows tests using remote_spawn to work with ssh.exp (rather than
> calling rsh with standard_spawn otherwise does by default).
> ---
>   lib/ssh.exp | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ssh.exp b/lib/ssh.exp
> index 5c2fbe0..bb827f4 100644
> --- a/lib/ssh.exp
> +++ b/lib/ssh.exp
> @@ -197,7 +197,7 @@ proc ssh_exec { boardname program pargs inp outp } {
>   }
>   
>   proc ssh_close { desthost } {
> -    global SSH ssh_initialized
> +    global SSH ssh_initialized board_info
>   
>       verbose "Closing the SSH connection to $desthost"
>   
> @@ -235,6 +235,62 @@ proc ssh_close { desthost } {
>      # Kill the remote server
>       set status [catch "exec ssh $ssh_port -o ControlPath=/tmp/ssh-%r@%h:%p -O exit $args"]
>       set ssh_initialized "no"
> +    if {[board_info $desthost exists fileid]} {
> +	set spawn_id [board_info $desthost fileid]
> +	unset board_info($desthost,fileid)
> +	catch "close -i $spawn_id"
> +	catch "wait -i $spawn_id"
> +    }
>   
>        return ""
>   }
> +
> +proc ssh_spawn { dest commandline } {
> +    global SSH timeout board_info
> +
> +    set ssh_port ""
> +    set scp_port ""
> +    set ssh_user ""
> +    set ssh_useropts ""
> +    set name ""
> +    set hostname ""
> +
> +    verbose "Spawning on $dest: $commandline"
> +
> +    if {![board_info $dest exists ssh_prog]} {
> +	set SSH ssh
> +    } else {
> +	set SSH [board_info $dest ssh_prog]
> +    }
> +
> +    if {[board_info $dest exists username]} {
> +	set ssh_user "[board_info $dest username]@"
> +    } else {
> +	set ssh_user ""
> +    }
> +
> +    if {[board_info $dest exists ssh_opts]} {
> +	append ssh_useropts " [board_info $dest ssh_opts]"
> +    }
> +
> +    if {[board_info $dest exists name]} {
> +	set dest [board_info $dest name]
> +    }
> +
> +    if {[board_info $dest exists hostname]} {
> +	set hostname [board_info $dest hostname]
> +    } else {
> +	set hostname $dest
> +    }
> +
> +    if {[board_info $dest exists port]} {
> +	append ssh_useropts " -p [board_info $dest port]"
> +    }
> +
> +    append ssh_useropts " -t -t -o ControlPersist=yes -o ControlMaster=auto -o ControlPath=\"/tmp/ssh-%r@%h:%p\""
> +
> +    spawn sh -c "$SSH $ssh_useropts $ssh_user$hostname '$commandline'"
> +
> +    set board_info($dest,fileid) $spawn_id
> +    return $spawn_id
> +}

I have a few quibbles with some of the above, but they are existing 
problems that will need to be cleaned up later.

The big problem I have here is an extra use of sh.  What prevents using 
Tcl list operations and {eval [list spawn $SSH] $ssh_useropts [list 
"$ssh_user$hostname" $commandline]} to move the argument splitting into 
Tcl and avoid an extra shell with all of its potential quoting pitfalls?

To use list operations here, initialize ssh_useropts to either 
[board_info $dest ssh_opts] or an empty list, then use lappend(n) to add 
to it.  This may also obviate the need for quoting ControlPath.

The main reason I do not want an extra shell used here is that it will 
break if $commandline ever contains single quotes.  It also wastes a 
process table slot, but that is a minor issue on modern systems and 
could be fixed by using exec(1) in the shell.


-- Jacob