diff --git a/Cargo.lock b/Cargo.lock index bc18c10..a6a7a8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -122,9 +122,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" [[package]] name = "libc" -version = "0.2.103" +version = "0.2.105" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd8f7255a17a627354f321ef0055d63b898c6fb27eff628af4d1b66b7331edf6" +checksum = "869d572136620d55835903746bcb5cdc54cb2851fd0aeec53220b4bb65ef3013" [[package]] name = "linked-hash-map" @@ -224,6 +224,26 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "thiserror" +version = "1.0.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "854babe52e4df1653706b98fcfc05843010039b406875930a70e4d9644e5c417" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa32fd3f627f367fe16f893e2597ae3c05020f8bba2666a4e6ea73d377e5714b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "unicode-segmentation" version = "1.8.0" @@ -263,7 +283,7 @@ dependencies = [ "bitflags", "libc", "vm-memory", - "vmm-sys-util 0.8.0", + "vmm-sys-util", ] [[package]] @@ -274,11 +294,12 @@ dependencies = [ "epoll", "libc", "log", + "thiserror", "vhost", "vhost-user-backend", "virtio-bindings", "vm-memory", - "vmm-sys-util 0.9.0", + "vmm-sys-util", ] [[package]] @@ -293,7 +314,7 @@ dependencies = [ "virtio-bindings", "virtio-queue", "vm-memory", - "vmm-sys-util 0.9.0", + "vmm-sys-util", ] [[package]] @@ -309,7 +330,7 @@ source = "git+https://github.com/rust-vmm/vm-virtio?rev=6013dd9#6013dd91b2e6eb77 dependencies = [ "log", "vm-memory", - "vmm-sys-util 0.8.0", + "vmm-sys-util", ] [[package]] @@ -323,16 +344,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "vmm-sys-util" -version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "01cf11afbc4ebc0d5c7a7748a77d19e2042677fc15faa2f4ccccb27c18a60605" -dependencies = [ - "bitflags", - "libc", -] - [[package]] name = "vmm-sys-util" version = "0.9.0" diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 0d2723f..c92044b 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 48.8, + "coverage_score": 46.8, "exclude_path": "", "crate_features": "" } 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 561e90d..b026de6 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -5,11 +5,13 @@ // // SPDX-License-Identifier: Apache-2.0 +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; @@ -20,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 @@ -147,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 { @@ -173,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)); } } } @@ -195,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; @@ -211,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, + )); } } @@ -246,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; @@ -275,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 { @@ -311,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 { @@ -323,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 { @@ -338,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 { @@ -354,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 { @@ -387,8 +412,7 @@ impl I2cAdapter { } _ => { - println!("Invalid SMBUS command: {}", smbus_data.size); - return Err(Error::new(EINVAL)); + return Err(Error::SMBusCommandInvalid(smbus_data.size)); } } } @@ -406,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<()> { @@ -452,7 +469,7 @@ impl I2cMap { device_map[*addr as usize] = i as u32; } - println!( + info!( "Added I2C master with bus id: {:x} for devices", adapter.adapter_no(), ); @@ -474,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. @@ -491,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, } @@ -507,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 } @@ -532,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(); @@ -617,6 +627,7 @@ pub mod tests { let mut i2c_map: I2cMap = I2cMap::new(&adapter_config).unwrap(); i2c_map.adapters[0].smbus = true; + // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation let mut reqs: Vec = vec![ I2cReq { addr: 0x3, @@ -632,12 +643,10 @@ pub mod tests { buf: [3, 4].to_vec(), }, ]; - - // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation - // TODO: check the actual error once we have an error type defined. - // TODO-continued: otherwise this test is unreliable because it might - // fail for another reason than the expected one. - assert!(i2c_map.transfer(&mut reqs).is_err()); + assert_eq!( + i2c_map.transfer(&mut reqs).unwrap_err(), + Error::SMBusTransferInvalid(2, 1, 2) + ); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation let mut reqs = vec![ @@ -655,7 +664,10 @@ pub mod tests { buf: [3, 4].to_vec(), }, ]; - assert!(i2c_map.transfer(&mut reqs).is_err()); + assert_eq!( + i2c_map.transfer(&mut reqs).unwrap_err(), + Error::SMBusTransferInvalid(2, 1, 2) + ); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation let mut reqs = vec![ @@ -673,7 +685,10 @@ pub mod tests { buf: [3, 4].to_vec(), }, ]; - assert!(i2c_map.transfer(&mut reqs).is_err()); + assert_eq!( + i2c_map.transfer(&mut reqs).unwrap_err(), + Error::SMBusTransferInvalid(2, 2, 2) + ); // I2C_SMBUS_READ (I2C_SMBUS_WORD_DATA) failure operation let mut reqs = vec![ @@ -691,6 +706,9 @@ pub mod tests { buf: [3, 4, 5].to_vec(), }, ]; - assert!(i2c_map.transfer(&mut reqs).is_err()); + assert_eq!( + i2c_map.transfer(&mut reqs).unwrap_err(), + Error::SMBusTransferInvalid(2, 1, 3) + ); } } diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index 768e4a0..9f9f2c4 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -8,11 +8,14 @@ mod i2c; 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}; @@ -20,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, @@ -34,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); @@ -66,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)); } } @@ -83,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)?; } @@ -115,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, @@ -143,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(); @@ -159,6 +182,14 @@ fn start_backend( let i2c_map = i2c_map.clone(); let handle = spawn(move || loop { + // A separate thread is spawned for each socket and can connect to a separate guest. + // These are run in an infinite loop to not require the daemon to be restarted once a + // guest exits. + // + // There isn't much value in complicating code here to return an error from the + // threads, and so the code uses unwrap() instead. The panic on a thread won't cause + // trouble to other threads/guests or the main() function and should be safe for the + // daemon. let backend = Arc::new(RwLock::new( VhostUserI2cBackend::new(i2c_map.clone()).unwrap(), )); @@ -175,38 +206,33 @@ fn start_backend( match daemon.wait() { Ok(()) => { - println!("Stopping cleanly."); + info!("Stopping cleanly."); } Err(vhost_user_backend::Error::HandleRequest( vhost_user::Error::PartialMessage, )) => { - println!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug."); + info!("vhost-user connection closed with partial message. If the VM is shutting down, this is expected behavior; otherwise, it might be a bug."); } Err(e) => { - println!("Error running daemon: {:?}", e); + warn!("Error running daemon: {:?}", e); } } // No matter the result, we need to shut down the worker thread. - backend - .read() - .unwrap() - .exit_event - .write(1) - .expect("Shutting down worker thread"); + backend.read().unwrap().exit_event.write(1).unwrap(); }); handles.push(handle); } 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(); @@ -251,21 +277,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) ); } @@ -295,4 +321,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 d605a6d..6c784da 100644 --- a/src/i2c/src/vhu_i2c.rs +++ b/src/i2c/src/vhu_i2c.rs @@ -5,10 +5,12 @@ // // SPDX-License-Identifier: Apache-2.0 +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}; @@ -26,36 +28,31 @@ 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, + #[error("Failed to create new EventFd: {0:?}")] + EventFdFailed(std::io::Error), } impl convert::From for io::Error { @@ -99,7 +96,7 @@ impl VhostUserI2cBackend { i2c_map, event_idx: false, mem: None, - exit_event: EventFd::new(EFD_NONBLOCK).expect("Creating exit eventfd"), + exit_event: EventFd::new(EFD_NONBLOCK).map_err(Error::EventFdFailed)?, }) } @@ -142,7 +139,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::() { @@ -209,14 +206,14 @@ impl VhostUserI2cBackend { } if vring.add_used(desc_chain.head_index(), len).is_err() { - println!("Couldn't return used descriptors to the ring"); + warn!("Couldn't return used descriptors to the ring"); } } // Send notification once all the requests are processed vring .signal_used_queue() - .map_err(|_| Error::DescriptorSendFailed)?; + .map_err(|_| Error::NotificationFailed)?; Ok(true) } } @@ -292,15 +289,15 @@ impl VhostUserBackendMut } _ => { - dbg!("unhandled device_event:", device_event); - return Err(Error::HandleEventUnknownEvent.into()); + warn!("unhandled device_event: {}", device_event); + return Err(Error::HandleEventUnknown.into()); } } Ok(false) } fn exit_event(&self, _thread_index: usize) -> Option { - Some(self.exit_event.try_clone().expect("Cloning exit eventfd")) + self.exit_event.try_clone().ok() } }