diff --git a/src/i2c/Cargo.toml b/src/i2c/Cargo.toml index 914d53f..49684ba 100644 --- a/src/i2c/Cargo.toml +++ b/src/i2c/Cargo.toml @@ -16,6 +16,7 @@ clap = { version = "=3.0.0-beta.2", features = ["yaml"] } epoll = "4.3" libc = ">=0.2.95" log = ">=0.4.6" +thiserror = "1.0" vhost = { version = "0.2", features = ["vhost-user-slave"] } vhost-user-backend = { git = "https://github.com/rust-vmm/vhost-user-backend", rev = "bd6b53348f06055abcb2b7254168d716b742f383" } virtio-bindings = ">=0.1" diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index d285849..e2380c5 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -9,8 +9,9 @@ use log::info; use std::fs::{File, OpenOptions}; use std::os::unix::io::AsRawFd; -use libc::{c_ulong, ioctl, EINVAL}; -use vmm_sys_util::errno::{errno_result, Error, Result}; +use libc::{c_ulong, ioctl}; +use thiserror::Error as ThisError; +use vmm_sys_util::errno::Error as IoError; use super::AdapterConfig; @@ -21,6 +22,27 @@ type IoctlRequest = libc::c_int; #[cfg(not(target_env = "musl"))] type IoctlRequest = c_ulong; +type Result = std::result::Result; + +#[derive(Copy, Clone, Debug, PartialEq, ThisError)] +/// Errors related to low level i2c helpers +pub enum Error { + #[error("Incorrect message length for {0} operation: {1}")] + MessageLengthInvalid(&'static str, usize), + #[error("Invalid SMBUS command: {0}")] + SMBusCommandInvalid(u32), + #[error("Invalid SMBus transfer, request-count: {0}, req[0].len: {1}, req[1].len: {2}")] + SMBusTransferInvalid(usize, u16, u16), + #[error("Failed to open adapter at /dev/i2c-{0}")] + DeviceOpenFailed(u32), + #[error("Ioctl command failed for {0} operation: {1}")] + IoctlFailure(&'static str, IoError), + #[error("Invalid Adapter function: {0:x}")] + AdapterFunctionInvalid(u64), + #[error("Invalid Client Address")] + ClientAddressInvalid, +} + /// Linux I2C/SMBUS definitions /// IOCTL commands @@ -148,11 +170,7 @@ impl I2cSmbusIoctlData { if (reqs[0].flags & I2C_M_RD) != 0 { // Special Read requests, reqs[0].len can be 0 or 1 only. if reqs[0].len > 1 { - println!( - "Incorrect message length for read operation: {}", - reqs[0].len - ); - return Err(Error::new(EINVAL)); + return Err(Error::MessageLengthInvalid("read", reqs[0].len as usize)); } read_write = I2C_SMBUS_READ; } else { @@ -174,11 +192,7 @@ impl I2cSmbusIoctlData { I2C_SMBUS_WORD_DATA } _ => { - println!( - "Message length not supported for write operation: {}", - reqs[0].len - ); - return Err(Error::new(EINVAL)); + return Err(Error::MessageLengthInvalid("write", reqs[0].len as usize)); } } } @@ -196,11 +210,11 @@ impl I2cSmbusIoctlData { || (reqs[0].len != 1) || (reqs[1].len > 2) { - println!( - "Expecting a valid read smbus transfer: {:?}", - (reqs.len(), reqs[0].len, reqs[1].len) - ); - return Err(Error::new(EINVAL)); + return Err(Error::SMBusTransferInvalid( + reqs.len(), + reqs[0].len, + reqs[1].len, + )); } read_write = I2C_SMBUS_READ; @@ -212,8 +226,11 @@ impl I2cSmbusIoctlData { } _ => { - println!("Invalid number of messages for smbus xfer: {}", reqs.len()); - return Err(Error::new(EINVAL)); + return Err(Error::SMBusTransferInvalid( + reqs.len(), + reqs[0].len, + reqs[1].len, + )); } } @@ -247,16 +264,16 @@ pub trait I2cDevice { Self: Sized; // Corresponds to the I2C_FUNCS ioctl call. - fn funcs(&mut self, func: u64) -> i32; + fn funcs(&mut self, func: u64) -> Result<()>; // Corresponds to the I2C_RDWR ioctl call. - fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32; + fn rdwr(&self, data: &I2cRdwrIoctlData) -> Result<()>; // Corresponds to the I2C_SMBUS ioctl call. - fn smbus(&self, data: &I2cSmbusIoctlData) -> i32; + fn smbus(&self, data: &I2cSmbusIoctlData) -> Result<()>; // Corresponds to the I2C_SLAVE ioctl call. - fn slave(&self, addr: u64) -> i32; + fn slave(&self, addr: u64) -> Result<()>; // Returns the adapter number corresponding to this device. fn adapter_no(&self) -> u32; @@ -276,25 +293,50 @@ impl I2cDevice for PhysDevice { file: OpenOptions::new() .read(true) .write(true) - .open(device_path)?, + .open(device_path) + .map_err(|_| Error::DeviceOpenFailed(adapter_no))?, adapter_no, }) } - fn funcs(&mut self, func: u64) -> i32 { - unsafe { ioctl(self.file.as_raw_fd(), I2C_FUNCS, &func) } + fn funcs(&mut self, func: u64) -> Result<()> { + let ret = unsafe { ioctl(self.file.as_raw_fd(), I2C_FUNCS, &func) }; + + if ret == -1 { + Err(Error::IoctlFailure("funcs", IoError::last())) + } else { + Ok(()) + } } - fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32 { - unsafe { ioctl(self.file.as_raw_fd(), I2C_RDWR, data) } + fn rdwr(&self, data: &I2cRdwrIoctlData) -> Result<()> { + let ret = unsafe { ioctl(self.file.as_raw_fd(), I2C_RDWR, data) }; + + if ret == -1 { + Err(Error::IoctlFailure("rdwr", IoError::last())) + } else { + Ok(()) + } } - fn smbus(&self, data: &I2cSmbusIoctlData) -> i32 { - unsafe { ioctl(self.file.as_raw_fd(), I2C_SMBUS, data) } + fn smbus(&self, data: &I2cSmbusIoctlData) -> Result<()> { + let ret = unsafe { ioctl(self.file.as_raw_fd(), I2C_SMBUS, data) }; + + if ret == -1 { + Err(Error::IoctlFailure("smbus", IoError::last())) + } else { + Ok(()) + } } - fn slave(&self, addr: u64) -> i32 { - unsafe { ioctl(self.file.as_raw_fd(), I2C_SLAVE, addr as c_ulong) } + fn slave(&self, addr: u64) -> Result<()> { + let ret = unsafe { ioctl(self.file.as_raw_fd(), I2C_SLAVE, addr as c_ulong) }; + + if ret == -1 { + Err(Error::IoctlFailure("slave", IoError::last())) + } else { + Ok(()) + } } fn adapter_no(&self) -> u32 { @@ -312,11 +354,8 @@ impl I2cAdapter { // Creates a new adapter corresponding to `device`. fn new(mut device: D) -> Result> { let func: u64 = I2C_FUNC_SMBUS_ALL; - let ret = device.funcs(func); - if ret == -1 { - println!("Failed to get I2C function"); - return errno_result(); - } + + device.funcs(func)?; let smbus; if (func & I2C_FUNC_I2C) != 0 { @@ -324,8 +363,7 @@ impl I2cAdapter { } else if (func & I2C_FUNC_SMBUS_ALL) != 0 { smbus = true; } else { - println!("Invalid functionality {:x}", func); - return Err(Error::new(EINVAL)); + return Err(Error::AdapterFunctionInvalid(func)); } Ok(I2cAdapter { @@ -339,7 +377,6 @@ impl I2cAdapter { fn i2c_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { let mut msgs: Vec = Vec::with_capacity(reqs.len()); let len = reqs.len(); - let addr = reqs[0].addr; for req in reqs { msgs.push(I2cMsg { @@ -355,27 +392,14 @@ impl I2cAdapter { nmsgs: len as u32, }; - let ret = self.device.rdwr(&data); - if ret == -1 { - println!("Failed to transfer i2c data to device addr to {:x}", addr); - errno_result() - } else { - Ok(()) - } + self.device.rdwr(&data) } /// Perform I2C_SMBUS transfer fn smbus_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { let smbus_data = I2cSmbusIoctlData::new(reqs)?; - let ret = self.device.smbus(&smbus_data); - if ret == -1 { - println!( - "Failed to transfer smbus data to device addr to {:x}", - reqs[0].addr - ); - return errno_result(); - } + self.device.smbus(&smbus_data)?; if smbus_data.read_write == I2C_SMBUS_READ { unsafe { @@ -388,8 +412,7 @@ impl I2cAdapter { } _ => { - println!("Invalid SMBUS command: {}", smbus_data.size); - return Err(Error::new(EINVAL)); + return Err(Error::SMBusCommandInvalid(smbus_data.size)); } } } @@ -407,14 +430,7 @@ impl I2cAdapter { /// Sets device's address for an I2C adapter. fn set_device_addr(&self, addr: usize) -> Result<()> { - let ret = self.device.slave(addr as u64); - - if ret == -1 { - println!("Failed to set device addr to {:x}", addr); - errno_result() - } else { - Ok(()) - } + self.device.slave(addr as u64) } fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> { @@ -475,7 +491,7 @@ impl I2cMap { // This can happen a lot while scanning the bus, don't print any errors. if index == I2C_INVALID_ADAPTER { - return Err(Error::new(EINVAL)); + return Err(Error::ClientAddressInvalid); } // get the corresponding adapter based on the device config. @@ -492,12 +508,12 @@ pub mod tests { use super::*; use std::convert::TryFrom; - #[derive(Debug, Default)] + #[derive(Debug)] pub struct DummyDevice { - funcs_result: i32, - rdwr_result: i32, - smbus_result: i32, - slave_result: i32, + funcs_result: Result<()>, + rdwr_result: Result<()>, + smbus_result: Result<()>, + slave_result: Result<()>, adapter_no: u32, } @@ -508,23 +524,26 @@ pub mod tests { { Ok(DummyDevice { adapter_no, - ..Default::default() + funcs_result: Ok(()), + rdwr_result: Ok(()), + smbus_result: Ok(()), + slave_result: Ok(()), }) } - fn funcs(&mut self, _func: u64) -> i32 { + fn funcs(&mut self, _func: u64) -> Result<()> { self.funcs_result } - fn rdwr(&self, _data: &I2cRdwrIoctlData) -> i32 { + fn rdwr(&self, _data: &I2cRdwrIoctlData) -> Result<()> { self.rdwr_result } - fn smbus(&self, _data: &I2cSmbusIoctlData) -> i32 { + fn smbus(&self, _data: &I2cSmbusIoctlData) -> Result<()> { self.smbus_result } - fn slave(&self, _addr: u64) -> i32 { + fn slave(&self, _addr: u64) -> Result<()> { self.slave_result } @@ -533,16 +552,6 @@ pub mod tests { } } - #[test] - fn test_i2c_map_duplicate_device4() { - assert!(AdapterConfig::try_from("1:4,2:32:21,5:4:23").is_err()); - } - - #[test] - fn test_duplicated_adapter_no() { - assert!(AdapterConfig::try_from("1:4,1:32:21,5:10:23").is_err()); - } - #[test] fn test_i2c_map() { let adapter_config = AdapterConfig::try_from("1:4,2:32:21,5:10:23").unwrap(); diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index c0712de..15a5cd4 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -10,10 +10,12 @@ mod vhu_i2c; use log::{info, warn}; use std::convert::TryFrom; +use std::num::ParseIntError; use std::sync::{Arc, RwLock}; use std::thread::spawn; use clap::{load_yaml, App, ArgMatches}; +use thiserror::Error as ThisError; use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; @@ -21,6 +23,31 @@ use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; use i2c::{I2cDevice, I2cMap, PhysDevice, MAX_I2C_VDEV}; use vhu_i2c::VhostUserI2cBackend; +type Result = std::result::Result; + +#[derive(Debug, PartialEq, ThisError)] +/// Errors related to low level i2c helpers +pub enum Error { + #[error("Invalid socket path")] + SocketPathInvalid, + #[error("Invalid socket count: {0}")] + SocketCountInvalid(usize), + #[error("Invalid device list")] + DeviceListInvalid, + #[error("Duplicate adapter detected: {0}")] + AdapterDuplicate(u32), + #[error("Invalid client address: {0}")] + ClientAddressInvalid(u16), + #[error("Duplicate client address detected: {0}")] + ClientAddressDuplicate(u16), + #[error("Low level I2c failure: {0:?}")] + I2cFailure(i2c::Error), + #[error("Failed while parsing to integer: {0:?}")] + ParseFailure(ParseIntError), + #[error("Failed to join threads")] + FailedJoiningThreads, +} + #[derive(Debug, PartialEq)] struct DeviceConfig { adapter_no: u32, @@ -35,13 +62,13 @@ impl DeviceConfig { } } - fn push(&mut self, addr: u16) -> std::result::Result<(), String> { + fn push(&mut self, addr: u16) -> Result<()> { if addr as usize > MAX_I2C_VDEV { - return Err(format!("Invalid address: {} (> maximum allowed)", addr)); + return Err(Error::ClientAddressInvalid(addr)); } if self.addr.contains(&addr) { - return Err(format!("Address already in use: {}", addr)); + return Err(Error::ClientAddressDuplicate(addr)); } self.addr.push(addr); @@ -67,14 +94,14 @@ impl AdapterConfig { self.inner.iter().any(|elem| elem.addr.contains(&addr)) } - fn push(&mut self, device: DeviceConfig) -> std::result::Result<(), String> { + fn push(&mut self, device: DeviceConfig) -> Result<()> { if self.contains_adapter_no(device.adapter_no) { - return Err("Duplicated adapter number".to_string()); + return Err(Error::AdapterDuplicate(device.adapter_no)); } for addr in device.addr.iter() { if self.contains_addr(*addr) { - return Err(format!("Address already in use: {}", addr)); + return Err(Error::ClientAddressDuplicate(*addr)); } } @@ -84,21 +111,19 @@ impl AdapterConfig { } impl TryFrom<&str> for AdapterConfig { - type Error = String; + type Error = Error; - fn try_from(list: &str) -> Result { + fn try_from(list: &str) -> Result { let busses: Vec<&str> = list.split(',').collect(); let mut devices = AdapterConfig::new(); for businfo in busses.iter() { let list: Vec<&str> = businfo.split(':').collect(); - let bus_addr = list[0].parse::().map_err(|_| "Invalid bus address")?; + let bus_addr = list[0].parse::().map_err(Error::ParseFailure)?; let mut adapter = DeviceConfig::new(bus_addr); for device_str in list[1..].iter() { - let addr = device_str - .parse::() - .map_err(|_| "Invalid device addr")?; + let addr = device_str.parse::().map_err(Error::ParseFailure)?; adapter.push(addr)?; } @@ -116,25 +141,27 @@ struct I2cConfiguration { } impl TryFrom for I2cConfiguration { - type Error = String; + type Error = Error; - fn try_from(cmd_args: ArgMatches) -> Result { + fn try_from(cmd_args: ArgMatches) -> Result { let socket_path = cmd_args .value_of("socket_path") - .ok_or("Invalid socket path")? + .ok_or(Error::SocketPathInvalid)? .to_string(); let socket_count = cmd_args .value_of("socket_count") .unwrap_or("1") .parse::() - .map_err(|_| "Invalid socket_count")?; + .map_err(Error::ParseFailure)?; if socket_count == 0 { - return Err("Socket count can't be 0".to_string()); + return Err(Error::SocketCountInvalid(0)); } - let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; + let list = cmd_args + .value_of("devices") + .ok_or(Error::DeviceListInvalid)?; let devices = AdapterConfig::try_from(list)?; Ok(I2cConfiguration { socket_path, @@ -144,14 +171,9 @@ impl TryFrom for I2cConfiguration { } } -fn start_backend( - config: I2cConfiguration, -) -> Result<(), String> { +fn start_backend(config: I2cConfiguration) -> Result<()> { // The same i2c_map structure instance is shared between all the guests - let i2c_map = Arc::new( - I2cMap::::new(&config.devices) - .map_err(|e| format!("Failed to create i2c_map ({})", e))?, - ); + let i2c_map = Arc::new(I2cMap::::new(&config.devices).map_err(Error::I2cFailure)?); let mut handles = Vec::new(); @@ -201,13 +223,13 @@ fn start_backend( } for handle in handles { - handle.join().map_err(|_| "Failed to join threads")?; + handle.join().map_err(|_| Error::FailedJoiningThreads)?; } Ok(()) } -fn main() -> Result<(), String> { +fn main() -> Result<()> { let yaml = load_yaml!("cli.yaml"); let cmd_args = App::from(yaml).get_matches(); @@ -252,21 +274,21 @@ mod tests { let cmd_args = get_cmd_args(socket_name, "1:4d", Some(5)); assert_eq!( I2cConfiguration::try_from(cmd_args).unwrap_err(), - "Invalid device addr" + Error::ParseFailure("4d".parse::().unwrap_err()) ); // Invalid socket count let cmd_args = get_cmd_args(socket_name, "1:4", Some(0)); assert_eq!( I2cConfiguration::try_from(cmd_args).unwrap_err(), - "Socket count can't be 0" + Error::SocketCountInvalid(0) ); // Duplicate client address: 4 let cmd_args = get_cmd_args(socket_name, "1:4,2:32:21,5:4:23", Some(5)); assert_eq!( I2cConfiguration::try_from(cmd_args).unwrap_err(), - "Address already in use: 4" + Error::ClientAddressDuplicate(4) ); } @@ -296,4 +318,20 @@ mod tests { assert_eq!(config, expected_config); } + + #[test] + fn test_i2c_map_duplicate_device4() { + assert_eq!( + AdapterConfig::try_from("1:4,2:32:21,5:4:23").unwrap_err(), + Error::ClientAddressDuplicate(4) + ); + } + + #[test] + fn test_duplicated_adapter_no() { + assert_eq!( + AdapterConfig::try_from("1:4,1:32:21,5:10:23").unwrap_err(), + Error::AdapterDuplicate(1) + ); + } } diff --git a/src/i2c/src/vhu_i2c.rs b/src/i2c/src/vhu_i2c.rs index 7b1132b..0d96204 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -8,8 +8,9 @@ use log::warn; use std::mem::size_of; use std::sync::Arc; -use std::{convert, error, fmt, io}; +use std::{convert, io}; +use thiserror::Error as ThisError; use vhost::vhost_user::message::{VhostUserProtocolFeatures, VhostUserVirtioFeatures}; use vhost_user_backend::{VhostUserBackendMut, VringRwLock, VringT}; use virtio_bindings::bindings::virtio_net::{VIRTIO_F_NOTIFY_ON_EMPTY, VIRTIO_F_VERSION_1}; @@ -27,36 +28,29 @@ const NUM_QUEUES: usize = 1; type Result = std::result::Result; type VhostUserBackendResult = std::result::Result; -#[derive(Debug)] +#[derive(Debug, ThisError)] /// Errors related to vhost-device-i2c daemon. pub enum Error { - /// Failed to handle event other than input event. + #[error("Failed to handle event, didn't match EPOLLIN")] HandleEventNotEpollIn, - /// Failed to handle unknown event. - HandleEventUnknownEvent, - /// Guest gave us a write only descriptor that protocol says to read from. + #[error("Failed to handle unknown event")] + HandleEventUnknown, + #[error("Received unexpected write only descriptor")] UnexpectedWriteOnlyDescriptor, - /// Guest gave us a readable descriptor that protocol says to only write to. - UnexpectedReadDescriptor, - /// Invalid descriptor count + #[error("Received unexpected readable descriptor")] + UnexpectedReadableDescriptor, + #[error("Invalid descriptor count")] UnexpectedDescriptorCount, - /// Invalid descriptor + #[error("Invalid descriptor size")] UnexpectedDescriptorSize, - /// Descriptor not found + #[error("Descriptor not found")] DescriptorNotFound, - /// Descriptor read failed + #[error("Descriptor read failed")] DescriptorReadFailed, - /// Descriptor write failed + #[error("Descriptor write failed")] DescriptorWriteFailed, - /// Descriptor send failed - DescriptorSendFailed, -} -impl error::Error for Error {} - -impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "vhost-device-i2c error: {:?}", self) - } + #[error("Failed to send notification")] + NotificationFailed, } impl convert::From for io::Error { @@ -143,7 +137,7 @@ impl VhostUserI2cBackend { let desc_in_hdr = descriptors[2]; if !desc_in_hdr.is_write_only() { - return Err(Error::UnexpectedReadDescriptor); + return Err(Error::UnexpectedReadableDescriptor); } if desc_in_hdr.len() as usize != size_of::() { @@ -217,7 +211,7 @@ impl VhostUserI2cBackend { // Send notification once all the requests are processed vring .signal_used_queue() - .map_err(|_| Error::DescriptorSendFailed)?; + .map_err(|_| Error::NotificationFailed)?; Ok(true) } } @@ -294,7 +288,7 @@ impl VhostUserBackendMut _ => { warn!("unhandled device_event: {}", device_event); - return Err(Error::HandleEventUnknownEvent.into()); + return Err(Error::HandleEventUnknown.into()); } } Ok(false)