Always return a Result when communicating with a device

Previously, we sometimes returned a value without wrapping it in a
result if the API method did not indicate errors in the return value.
But we can detect errors using the NK_get_last_command_status function.
This patch changes the return types of these methods to Result<_, Error>
and adds error checks.

4 files changed, 33 insertions, 20 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 24c79af..25a8c31 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -31,6 +31,7 @@ SPDX-License-Identifier: MIT
   - Return `Error::Utf8Error` if libnitrokey returns an invalid UTF-8 string.
 - Implement `From<(T: Device, Error)>` for `Error`.
 - Fix timing issues with the `totp_no_pin` and `totp_pin` test cases.
+- Always return a `Result` in functions that communicate with a device.
 
 # v0.3.4 (2019-01-20)
 - Fix authentication methods that assumed that `char` is signed.
diff --git a/src/device.rs b/src/device.rs
index 386ce94..4178922 100644
--- a/src/device.rs
+++ b/src/device.rs
@@ -12,7 +12,9 @@ use crate::config::{Config, RawConfig};
 use crate::error::{CommunicationError, Error};
 use crate::otp::GenerateOtp;
 use crate::pws::GetPasswordSafe;
-use crate::util::{get_command_result, get_cstring, get_last_error, result_from_string};
+use crate::util::{
+    get_command_result, get_cstring, get_last_error, result_from_string, result_or_error,
+};
 
 /// Available Nitrokey models.
 #[derive(Clone, Copy, Debug, PartialEq)]
@@ -343,13 +345,15 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug {
     ///
     /// # fn try_main() -> Result<(), Error> {
     /// let device = nitrokey::connect()?;
-    /// let count = device.get_user_retry_count();
-    /// println!("{} remaining authentication attempts (user)", count);
+    /// match device.get_user_retry_count() {
+    ///     Ok(count) => println!("{} remaining authentication attempts (user)", count),
+    ///     Err(err) => println!("Could not get user retry count: {}", err),
+    /// }
     /// #     Ok(())
     /// # }
     /// ```
-    fn get_user_retry_count(&self) -> u8 {
-        unsafe { nitrokey_sys::NK_get_user_retry_count() }
+    fn get_user_retry_count(&self) -> Result<u8, Error> {
+        result_or_error(unsafe { nitrokey_sys::NK_get_user_retry_count() })
     }
 
     /// Returns the number of remaining authentication attempts for the admin.  The total number of
@@ -364,12 +368,15 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug {
     /// # fn try_main() -> Result<(), Error> {
     /// let device = nitrokey::connect()?;
     /// let count = device.get_admin_retry_count();
-    /// println!("{} remaining authentication attempts (admin)", count);
+    /// match device.get_admin_retry_count() {
+    ///     Ok(count) => println!("{} remaining authentication attempts (admin)", count),
+    ///     Err(err) => println!("Could not get admin retry count: {}", err),
+    /// }
     /// #     Ok(())
     /// # }
     /// ```
-    fn get_admin_retry_count(&self) -> u8 {
-        unsafe { nitrokey_sys::NK_get_admin_retry_count() }
+    fn get_admin_retry_count(&self) -> Result<u8, Error> {
+        result_or_error(unsafe { nitrokey_sys::NK_get_admin_retry_count() })
     }
 
     /// Returns the major part of the firmware version (should be zero).
@@ -384,14 +391,14 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug {
     /// let device = nitrokey::connect()?;
     /// println!(
     ///     "Firmware version: {}.{}",
-    ///     device.get_major_firmware_version(),
-    ///     device.get_minor_firmware_version(),
+    ///     device.get_major_firmware_version().unwrap(),
+    ///     device.get_minor_firmware_version().unwrap(),
     /// );
     /// #     Ok(())
     /// # }
     /// ```
-    fn get_major_firmware_version(&self) -> i32 {
-        unsafe { nitrokey_sys::NK_get_major_firmware_version() }
+    fn get_major_firmware_version(&self) -> Result<i32, Error> {
+        result_or_error(unsafe { nitrokey_sys::NK_get_major_firmware_version() })
     }
 
     /// Returns the minor part of the firmware version (for example 8 for version 0.8).
@@ -406,13 +413,13 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug {
     /// let device = nitrokey::connect()?;
     /// println!(
     ///     "Firmware version: {}.{}",
-    ///     device.get_major_firmware_version(),
-    ///     device.get_minor_firmware_version(),
+    ///     device.get_major_firmware_version().unwrap(),
+    ///     device.get_minor_firmware_version().unwrap(),
     /// );
     /// #     Ok(())
     /// # }
-    fn get_minor_firmware_version(&self) -> i32 {
-        unsafe { nitrokey_sys::NK_get_minor_firmware_version() }
+    fn get_minor_firmware_version(&self) -> Result<i32, Error> {
+        result_or_error(unsafe { nitrokey_sys::NK_get_minor_firmware_version() })
     }
 
     /// Returns the current configuration of the Nitrokey device.
diff --git a/src/util.rs b/src/util.rs
index b7e8cd3..fdb73c3 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -53,6 +53,10 @@ pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> {
     }
 }
 
+pub fn result_or_error<T>(value: T) -> Result<T, Error> {
+    get_last_result().and(Ok(value))
+}
+
 pub fn get_command_result(value: c_int) -> Result<(), Error> {
     if value == 0 {
         Ok(())
diff --git a/tests/device.rs b/tests/device.rs
index c790049..7ab4d66 100644
--- a/tests/device.rs
+++ b/tests/device.rs
@@ -95,9 +95,10 @@ fn get_serial_number(device: DeviceWrapper) {
 }
 #[test_device]
 fn get_firmware_version(device: Pro) {
-    assert_eq!(0, device.get_major_firmware_version());
+    assert_ok!(0, device.get_major_firmware_version());
     let minor = device.get_minor_firmware_version();
-    assert!(minor > 0);
+    assert!(minor.is_ok());
+    assert!(minor.unwrap() > 0);
 }
 
 fn admin_retry<T: Authenticate + Device>(device: T, suffix: &str, count: u8) -> T {
@@ -106,7 +107,7 @@ fn admin_retry<T: Authenticate + Device>(device: T, suffix: &str, count: u8) ->
         Ok(admin) => admin.device(),
         Err((device, _)) => device,
     };
-    assert_eq!(count, device.get_admin_retry_count());
+    assert_ok!(count, device.get_admin_retry_count());
     return device;
 }
 
@@ -116,7 +117,7 @@ fn user_retry<T: Authenticate + Device>(device: T, suffix: &str, count: u8) -> T
         Ok(admin) => admin.device(),
         Err((device, _)) => device,
     };
-    assert_eq!(count, device.get_user_retry_count());
+    assert_ok!(count, device.get_user_retry_count());
     return device;
 }