tighten up `Session::rc`

Previously, this function could return None even if passed an error
result.  This could happen in situations where no error had been
recorded against the session object, but where libssh2 had returned
an error condition anyway.

It was also possible for the prior implementation to return the
wrong information in a similar situation: if an error had been
recorded against the session, that error would be returned rather
than the current error code.

This commit tidies up the implementation of this function by
always returning an `Error` object that corresponds to the
provided `rc` parameter.  If the last error message recorded
against the `Session` has a matching error value then that
error message will be used, otherwise we'll synthesize one
just from the error code.

2 files changed, 31 insertions, 25 deletions

diff --git a/src/error.rs b/src/error.rs
index ade9001..c5b7543 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -3,6 +3,7 @@ use std::error;
 use std::ffi::NulError;
 use std::fmt;
 use std::io;
+use std::ptr::null_mut;
 use std::str;
 
 use {raw, Session};
@@ -30,6 +31,30 @@ impl Error {
         }
     }
 
+    /// Given a libssh2 error return code, generate an Error object that
+    /// encapsulates that error code and the error reason.
+    /// The error reason is extracted from the Session and is used if the
+    /// session contains the same error code as that provided.
+    /// If the error code doesn't match then an approximation of the error
+    /// reason is used instead of the error message stored in the Session.
+    pub fn from_session_error(sess: &Session, rc: libc::c_int) -> Error {
+        Self::from_session_error_raw(sess.raw(), rc)
+    }
+
+    #[doc(hidden)]
+    pub fn from_session_error_raw(raw: *mut raw::LIBSSH2_SESSION, rc: libc::c_int) -> Error {
+        static STATIC: () = ();
+        unsafe {
+            let mut msg = null_mut();
+            let res = raw::libssh2_session_last_error(raw, &mut msg, null_mut(), 0);
+            if res != rc {
+                return Self::from_errno(rc);
+            }
+            let s = ::opt_bytes(&STATIC, msg).unwrap();
+            Error::new(rc, str::from_utf8(s).unwrap())
+        }
+    }
+
     /// Generate the last error that occurred for a `Session`.
     ///
     /// Returns `None` if there was no last error.
diff --git a/src/session.rs b/src/session.rs
index a8e5dc3..d0049c5 100644
--- a/src/session.rs
+++ b/src/session.rs
@@ -230,16 +230,7 @@ impl Session {
                 )
             })?;
 
-            let res = handshake(self.inner.raw, stream);
-            self.rc(res)?;
-            if res < 0 {
-                // There are some kex related errors that don't set the
-                // last error on the session object and that will not cause
-                // the `rc` function to emit an error.
-                // Let's ensure that we indicate an error in this situation.
-                return Err(Error::new(res, "Error during handshake"));
-            }
-            Ok(())
+            self.rc(handshake(self.inner.raw, stream))
         }
     }
 
@@ -601,10 +592,10 @@ impl Session {
             let mut ptr = 0 as *mut _;
             let rc = raw::libssh2_session_supported_algs(self.inner.raw, method_type, &mut ptr);
             if rc <= 0 {
-                try!(self.rc(rc))
+                self.rc(rc)?;
             }
             for i in 0..(rc as isize) {
-                let s = ::opt_bytes(&STATIC, *ptr.offset(i)).unwrap();;
+                let s = ::opt_bytes(&STATIC, *ptr.offset(i)).unwrap();
                 let s = str::from_utf8(s).unwrap();
                 ret.push(s);
             }
@@ -881,7 +872,7 @@ impl Session {
     pub fn keepalive_send(&self) -> Result<u32, Error> {
         let mut ret = 0;
         let rc = unsafe { raw::libssh2_keepalive_send(self.inner.raw, &mut ret) };
-        try!(self.rc(rc));
+        self.rc(rc)?;
         Ok(ret as u32)
     }
 
@@ -912,14 +903,7 @@ impl Session {
 
     /// Translate a return code into a Rust-`Result`.
     pub fn rc(&self, rc: c_int) -> Result<(), Error> {
-        if rc >= 0 {
-            Ok(())
-        } else {
-            match Error::last_error(self) {
-                Some(e) => Err(e),
-                None => Ok(()),
-            }
-        }
+        self.inner.rc(rc)
     }
 }
 
@@ -929,10 +913,7 @@ impl SessionInner {
         if rc >= 0 {
             Ok(())
         } else {
-            match Error::last_error_raw(self.raw) {
-                Some(e) => Err(e),
-                None => Ok(()),
-            }
+            Err(Error::from_session_error_raw(self.raw, rc))
         }
     }
 }