From c30cbd35ba187cd6e5055d3beb8420b11fb030ec Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Sun, 27 Jan 2019 23:23:00 +0000 Subject: 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. --- CHANGELOG.md | 1 + src/device.rs | 39 +++++++++++++++++++++++---------------- src/util.rs | 4 ++++ tests/device.rs | 9 +++++---- 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 { + 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 { + 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 { + 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 { + 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 { } } +pub fn result_or_error(value: T) -> Result { + 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(device: T, suffix: &str, count: u8) -> T { @@ -106,7 +107,7 @@ fn admin_retry(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(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; } -- cgit v1.2.1