Merge pull request #25 from JuliaWeb/sftp

Various fixes

Author: JamesWrigley

docs/src/changelog.md

@@ -15,10 +15,18 @@ Changelog](https://keepachangelog.com).
   construction ([#21]).
 - Our [`Base.run()`](@ref) methods now accept plain `String`s as well as `Cmd`s
   ([#24]).
+- Implemented convenience [`Base.read(::String, ::SftpSession)`](@ref) methods
+  that will take a `String` filename without having to open the file explicitly
+  ([#25]).
+- Added support for specifying whether a [`Session`](@ref) should use the users
+  SSH config with the `process_config` option ([#25]).
 
 ### Fixed
 
 - Improved handling of possible errors in [`Base.readdir()`](@ref) ([#20]).
+- Fixed exception handling for [`Base.run()`](@ref), now it throws a
+  [`SshProcessFailedException`](@ref) or [`LibSSHException`](@ref) on command
+  failure instead of a plain `TaskFailedException` ([#25]).
 
 ## [v0.6.0] - 2024-10-11
 

docs/src/sessions_and_channels.md

@@ -111,11 +111,11 @@ SshProcessFailedException
 SshProcess
 Base.wait(::SshProcess)
 Base.success(::SshProcess)
-Base.run(::Cmd, ::Session)
-Base.read(::Cmd, ::Session)
-Base.read(::Cmd, ::Session, ::Type{String})
-Base.readchomp(::Cmd, ::Session)
-Base.success(::Cmd, ::Session)
+Base.run(::Union{Cmd, String}, ::Session)
+Base.read(::Union{Cmd, String}, ::Session)
+Base.read(::Union{Cmd, String}, ::Session, ::Type{String})
+Base.readchomp(::Union{Cmd, String}, ::Session)
+Base.success(::Union{Cmd, String}, ::Session)
 ```
 
 #### Direct port forwarding

docs/src/sftp.md

@@ -59,7 +59,9 @@ Base.open(::String, ::SftpSession)
 Base.open(::Function, ::String, ::SftpSession)
 Base.close(::SftpFile)
 Base.read(::SftpFile)
+Base.read(::String, ::SftpSession)
 Base.read(::SftpFile, ::Type{String})
+Base.read(::String, ::SftpSession, ::Type{String})
 Base.read!(::SftpFile, ::Vector{UInt8})
 Base.write(::SftpFile, ::DenseVector)
 Base.write(::SftpFile, ::AbstractString)

src/channel.jl

@@ -493,8 +493,18 @@ $(TYPEDSIGNATURES)
 function Base.wait(process::SshProcess)
     try
         wait(process._task)
-    catch ex
-        if process.cmd isa Cmd && !process.cmd.ignorestatus
+    catch task_ex
+        ex = process._task.exception
+
+        # The idea is that SshProcessFailedException's and LibSSHException's are
+        # somewhat expected so we always unwrap them from the
+        # TaskFailedException before throwing, which is a slightly nicer API to
+        # work with.
+        if ex isa SshProcessFailedException || ex isa LibSSHException
+            if !(process.cmd isa Cmd && process.cmd.ignorestatus)
+                throw(process._task.exception)
+            end
+        else
             rethrow()
         end
     end
@@ -591,6 +601,8 @@ An easy way of getting around these restrictions is to pass the command as a
 # Throws
 - [`SshProcessFailedException`](@ref): if the command fails and `ignorestatus()`
   wasn't used.
+- [`LibSSHException`](@ref): if running the command fails for some other
+  reason.
 
 # Arguments
 - `cmd`: The command to run. This will be converted to a string for running
@@ -652,10 +664,10 @@ function Base.run(cmd::Union{Cmd, String}, session::Session;
     set_channel_callbacks(process._sshchan, callbacks)
 
     process._task = Threads.@spawn _exec_command(process)
-    errormonitor(process._task)
+
     if wait
         # Note the use of Base.wait() to avoid aliasing with the `wait` argument
-        Base.wait(process._task)
+        Base.wait(process)
 
         if print_out
             print(String(copy(process.out)))

src/session.jl

@@ -26,6 +26,7 @@ mutable struct Session
     ssh_dir::Union{String, Nothing}
     known_hosts::Union{String, Nothing}
     gssapi_server_identity::Union{String, Nothing}
+    process_config::Bool
 
     _lock::ReentrantLock
     _auth_methods::Union{Vector{AuthMethod}, Nothing}
@@ -68,7 +69,7 @@ mutable struct Session
         lib.ssh_set_blocking(ptr, 0)
 
         session = new(ptr, own, [], nothing,
-                      -1, nothing, nothing, nothing,
+                      -1, nothing, nothing, nothing, true,
                       ReentrantLock(), nothing, AuthMethod[], true,
                       Threads.Event(true), CloseableCondition(), false)
 
@@ -137,6 +138,13 @@ $(TYPEDSIGNATURES)
 Constructor for creating a client session. Use this if you want to connect to a
 server.
 
+!!! warning
+    By default libssh will try to follow the settings in any found SSH config
+    files. If a proxyjump is configured for `host` libssh will try to set up the
+    proxy itself, which usually does not play well with Julia's event loop. In
+    such situations you will probably want to pass `process_config=false` and
+    set up the proxyjump explicitly using a [`Forwarder`](@ref).
+
 # Throws
 - [`LibSSHException`](@ref): if a session couldn't be created, or there was an
   error initializing the `user` property.
@@ -152,6 +160,7 @@ server.
 - `log_verbosity=nothing`: Set the log verbosity for the session.
 - `auto_connect=true`: Whether to automatically call
   [`connect()`](@ref).
+- `process_config=true`: Whether to process any found SSH config files.
 
 # Examples
 
@@ -163,7 +172,8 @@ julia> session = ssh.Session(ip"12.34.56.78", 2222)
 """
 function Session(host::Union{AbstractString, Sockets.IPAddr}, port=22;
                  socket::Union{Sockets.TCPSocket, RawFD, Nothing}=nothing,
-                 user=nothing, log_verbosity=nothing, auto_connect=true)
+                 user=nothing, log_verbosity=nothing, auto_connect=true,
+                 process_config=true)
     session_ptr = lib.ssh_new()
     if session_ptr == C_NULL
         throw(LibSSHException("Could not initialize Session for host $(host)"))
@@ -192,6 +202,8 @@ function Session(host::Union{AbstractString, Sockets.IPAddr}, port=22;
             session.user = user
         end
 
+        session.process_config = process_config
+
         if auto_connect
             connect(session)
         end
@@ -295,10 +307,11 @@ const SESSION_PROPERTY_OPTIONS = Dict(:host => (SSH_OPTIONS_HOST, Cstring),
                                       :ssh_dir => (SSH_OPTIONS_SSH_DIR, Cstring),
                                       :known_hosts => (SSH_OPTIONS_KNOWNHOSTS, Cstring),
                                       :gssapi_server_identity => (SSH_OPTIONS_GSSAPI_SERVER_IDENTITY, Cstring),
-                                      :log_verbosity => (SSH_OPTIONS_LOG_VERBOSITY, Cuint))
+                                      :log_verbosity => (SSH_OPTIONS_LOG_VERBOSITY, Cuint),
+                                      :process_config => (SSH_OPTIONS_PROCESS_CONFIG, Bool))
 # These properties cannot be retrieved from the libssh API (i.e. with
 # ssh_options_get()), so we store them in the Session object instead.
-const SAVED_PROPERTIES = (:log_verbosity, :gssapi_server_identity, :ssh_dir, :known_hosts)
+const SAVED_PROPERTIES = (:log_verbosity, :gssapi_server_identity, :ssh_dir, :known_hosts, :process_config)
 
 function Base.propertynames(::Session, private::Bool=false)
     fields = fieldnames(Session)

src/sftp.jl

@@ -142,8 +142,8 @@ mutable struct SftpSession
     - [`LibSSHException`](@ref): If creating the SFTP session fails.
     """
     function SftpSession(session::Session)
-        if !isopen(session)
-            throw(ArgumentError("Session is closed, cannot create an SftpSession with it"))
+        if !isconnected(session)
+            throw(ArgumentError("Session is disconnected, cannot create an SftpSession with it"))
         end
 
         ptr = @lockandblock session lib.sftp_new(session.ptr)
@@ -907,6 +907,9 @@ function Base.read(file::SftpFile, nb::Integer=typemax(Int))
     return out
 end
 
+"$(TYPEDSIGNATURES)"
+Base.read(filename::AbstractString, sftp::SftpSession) = open(read, filename, sftp)
+
 """
 $(TYPEDSIGNATURES)
 
@@ -973,6 +976,10 @@ Read the whole file as a `String`.
 """
 Base.read(file::SftpFile, ::Type{String}) = String(read(file))
 
+"$(TYPEDSIGNATURES)"
+Base.read(filename::AbstractString, sftp::SftpSession, ::Type{String}) = String(read(filename, sftp))
+
+
 """
 $(TYPEDSIGNATURES)
 

test/LibSSHTests.jl

@@ -355,6 +355,8 @@ end
         session.gssapi_server_identity = "foo.com"
         @test session.gssapi_server_identity == "foo.com"
         @test session.fd == RawFD(-1)
+        session.process_config = false
+        @test !session.process_config
 
         # Test setting an initial user
         ssh.Session("localhost"; user="foo", auto_connect=false) do session2
@@ -562,16 +564,8 @@ end
             cmd = setenv(`echo \$foo`, "foo" => "bar")
             @test readchomp(cmd, session) == "bar"
 
-            # Test command failure. We redirect error output to devnull to hide
-            # the errors displayed by the errormonitor() task.
-            try
-                redirect_stderr(devnull) do
-                    run(`foo`, session)
-                end
-            catch ex
-                @test ex isa TaskFailedException
-                @test current_exceptions(ex.task)[1][1] isa ssh.SshProcessFailedException
-            end
+            # Test command failure
+            @test_throws ssh.SshProcessFailedException run(`foo`, session)
 
             # Test passing a String instead of a Cmd
             mktempdir() do tmpdir
@@ -773,6 +767,10 @@ end
                 # Shouldn't be able to read from a closed file
                 close(file)
                 @test_throws ArgumentError read(file)
+
+                # Test reading by passing just a filename
+                write(path, msg)
+                @test read(path, sftp, String) == msg
             end
         end
     end