From 162e1adc4fbbeb6a63df518ef6fe355e9095e27f Mon Sep 17 00:00:00 2001 From: Andreea Florescu Date: Wed, 15 Sep 2021 18:07:49 +0300 Subject: [PATCH] [i2c] separate parsing of strings from device This separation is needed so we can easily write unit tests for parsing without needing to create full blown objects. (as a side note, before this is possible, we also need to get rid of errno, and replace it with custom Errors, so that we can also write the much needed negative tests). This separation is achieved through creating configuration structures that can be either programatically initialized, or initialized through parsing the command line parameters. This commit is still WIP because we also need to make sure that configuration objects can only be created valid (to reduce some risks for future extensions where parameters might be passed some other way rather than yaml). Also, we need to move the check for uniquness of device addresses in the DeviceConfig instead of the `I2cMap`. Signed-off-by: Andreea Florescu --- src/i2c/src/i2c.rs | 46 ++++++++++++------------- src/i2c/src/main.rs | 82 +++++++++++++++++++++++++++++++++++---------- 2 files changed, 87 insertions(+), 41 deletions(-) diff --git a/src/i2c/src/i2c.rs b/src/i2c/src/i2c.rs index f4e28d1..3815219 100644 --- a/src/i2c/src/i2c.rs +++ b/src/i2c/src/i2c.rs @@ -416,7 +416,7 @@ impl I2cAdapter { } /// I2C map and helpers -const MAX_I2C_VDEV: usize = 1 << 7; +pub(crate) const MAX_I2C_VDEV: usize = 1 << 7; const I2C_INVALID_ADAPTER: u32 = 0xFFFFFFFF; pub struct I2cMap { @@ -424,46 +424,46 @@ pub struct I2cMap { device_map: [u32; MAX_I2C_VDEV], } +pub(crate) struct DeviceConfig { + pub(crate) adapter_no: u32, + pub(crate) addr: Vec, +} + +pub(crate) struct I2cConfiguration { + pub(crate) socket_path: String, + pub(crate) socket_count: usize, + pub(crate) devices: Vec, +} + impl I2cMap { - pub fn new(list: &str) -> Result + pub(crate) fn new(list: &Vec) -> Result where Self: Sized, { let mut device_map: [u32; MAX_I2C_VDEV] = [I2C_INVALID_ADAPTER; MAX_I2C_VDEV]; let mut adapters: Vec> = Vec::new(); - let busses: Vec<&str> = list.split(',').collect(); - for (i, businfo) in busses.iter().enumerate() { - let list: Vec<&str> = businfo.split(':').collect(); - let adapter_no = list[0].parse::().map_err(|_| Error::new(EINVAL))?; - let mut adapter = I2cAdapter::new(adapter_no)?; - let devices = &list[1..]; + for (i, device_cfg) in list.iter().enumerate() { + let adapter = I2cAdapter::new(device_cfg.adapter_no)?; - for device in devices { - let device = device.parse::().map_err(|_| Error::new(EINVAL))?; - - if device > MAX_I2C_VDEV { - println!("Invalid device address {}", device); - return Err(Error::new(EADDRNOTAVAIL)); - } - - if device_map[device] != I2C_INVALID_ADAPTER { + for addr in &device_cfg.addr { + if device_map[*addr as usize] != I2C_INVALID_ADAPTER { println!( "Client address {} is already used by {}", - device, - adapters[device_map[device] as usize].adapter_no() + *addr, + adapters[device_map[*addr as usize] as usize].adapter_no() ); return Err(Error::new(EADDRINUSE)); } - adapter.set_device_addr(device)?; - device_map[device] = i as u32; + // Calling this to check that the `addr` is valid. + adapter.set_device_addr(*addr)?; + device_map[*addr as usize] = i as u32; } println!( - "Added I2C master with bus id: {:x} for devices: {:?}", + "Added I2C master with bus id: {:x} for devices", adapter.adapter_no(), - devices ); adapters.push(adapter); diff --git a/src/i2c/src/main.rs b/src/i2c/src/main.rs index e4ddea2..d108df5 100644 --- a/src/i2c/src/main.rs +++ b/src/i2c/src/main.rs @@ -8,6 +8,8 @@ mod i2c; mod vhu_i2c; +use std::convert::TryFrom; +use std::num::ParseIntError; use std::sync::{Arc, RwLock}; use std::thread::spawn; @@ -16,11 +18,63 @@ use vhost::{vhost_user, vhost_user::Listener}; use vhost_user_backend::VhostUserDaemon; use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap}; -use i2c::{I2cDevice, I2cMap, PhysDevice}; -use std::convert::TryFrom; -use std::num::ParseIntError; +use i2c::{DeviceConfig, I2cConfiguration, I2cDevice, I2cMap, PhysDevice, MAX_I2C_VDEV}; use vhu_i2c::VhostUserI2cBackend; +impl TryFrom for I2cConfiguration { + type Error = String; + + fn try_from(cmd_args: ArgMatches) -> Result { + let socket_path = cmd_args + .value_of("socket_path") + .ok_or("Invalid socket path")? + .to_string(); + + let socket_count = cmd_args + .value_of("socket_count") + .unwrap_or("1") + .parse::() + .map_err(|_| "Invalid socket_count")?; + + let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; + let busses: Vec<&str> = list.split(',').collect(); + + let mut devices = Vec::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_devices = list[1..] + .iter() + .map(|str| str.parse::()) + .collect::, ParseIntError>>() + .map_err(|_| "Invalid device")?; + + // Check if any of the devices has a size > the maximum allowed one. + if bus_devices + .iter() + .filter(|addr| **addr > MAX_I2C_VDEV) + .count() + > 0 + { + // TODO: if needed we can show which one is actually not respecting the max size. + return Err("Invalid addr.".to_string()); + } + + devices.push(DeviceConfig { + adapter_no: bus_addr, + addr: bus_devices, + }) + } + + Ok(I2cConfiguration { + socket_path, + socket_count, + devices, + }) + } +} + fn start_daemon( backend: Arc>>, listener: Listener, @@ -63,24 +117,16 @@ fn start_backend( ) -> Result<(), String> { let mut handles = Vec::new(); - let path = cmd_args - .value_of("socket_path") - .ok_or("Invalid socket path")?; - - let count = cmd_args - .value_of("socket_count") - .unwrap_or("1") - .parse::() - .map_err(|_| "Invalid socket_count")?; - - let list = cmd_args.value_of("devices").ok_or("Invalid devices list")?; + let i2c_config = I2cConfiguration::try_from(cmd_args)?; // The same i2c_map structure instance is shared between all the guests - let i2c_map = - Arc::new(I2cMap::::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?); + let i2c_map = Arc::new( + I2cMap::::new(&i2c_config.devices) + .map_err(|e| format!("Failed to create i2c_map ({})", e))?, + ); - for i in 0..count { - let socket = path.to_owned() + &i.to_string(); + for i in 0..i2c_config.socket_count { + let socket = i2c_config.socket_path.to_owned() + &i.to_string(); let i2c_map = i2c_map.clone(); let handle = spawn(move || loop {