From ee173d6a7819d6a46ce74ebb6e166d1b5a4a7076 Mon Sep 17 00:00:00 2001 From: ConcurrentCrab Date: Sun, 16 Jul 2023 22:58:46 +0530 Subject: [PATCH] keyboard_states: use HashMap keyboard_stream_map and keyboard_states have to be kept in sync, but beacause of states being a vector, it had different semantics during removal. If 2 devices were added and the first was removed then the vector shifts the second device to the front but the map obviously did not, leading to the collections becoming out of sync. This cause out-of-bounds errors on the states vector when accessing it using stream_map's keys. This patch changes keyboard_states to be a map as well, and both maps are indexed using the udev input path as a key. A string key is not ideal, but is close to being the only unique ID for a connected device (physical path is not made serialised in many udev events for reasons unknown.), besides being easily accessible from both disjoint APIs being used here, evdev and tokio_udev. An additional concern may be that tokio's StreamMap manages its own members and removes streams upon completion, but: a) We don't index into stream_map at all, only consume from it, so this will never be an issue b) the corresponding entry in states will be removed as well upon the udev remove event --- swhkd/src/daemon.rs | 85 +++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/swhkd/src/daemon.rs b/swhkd/src/daemon.rs index c1c5386..d0cffd3 100644 --- a/swhkd/src/daemon.rs +++ b/swhkd/src/daemon.rs @@ -145,14 +145,13 @@ async fn main() -> Result<(), Box> { let arg_devices: Vec<&str> = args.values_of("device").unwrap_or_default().collect(); - let keyboard_devices: Vec = { + let keyboard_devices: Vec<_> = { if arg_devices.is_empty() { log::trace!("Attempting to find all keyboard file descriptors."); - evdev::enumerate().map(|(_, device)| device).filter(check_device_is_keyboard).collect() + evdev::enumerate().filter(|(_, dev)| { check_device_is_keyboard(dev) }).collect() } else { evdev::enumerate() - .map(|(_, device)| device) - .filter(|device| arg_devices.contains(&device.name().unwrap_or(""))) + .filter(|(_, dev)| arg_devices.contains(&dev.name().unwrap_or(""))) .collect() } }; @@ -212,13 +211,17 @@ async fn main() -> Result<(), Box> { let mut execution_is_paused = false; let mut last_hotkey: Option = None; let mut pending_release: bool = false; - let mut keyboard_states: Vec = Vec::new(); + let mut keyboard_states = HashMap::new(); let mut keyboard_stream_map = StreamMap::new(); - for (i, mut device) in keyboard_devices.into_iter().enumerate() { + for (path, mut device) in keyboard_devices.into_iter() { let _ = device.grab(); - keyboard_stream_map.insert(i, device.into_event_stream()?); - keyboard_states.push(KeyboardState::new()); + let path = match path.to_str() { + Some(p) => p, + None => { continue; }, + }; + keyboard_states.insert(path.to_string(), KeyboardState::new()); + keyboard_stream_map.insert(path.to_string(), device.into_event_stream()?); } // The initial sleep duration is never read because last_hotkey is initialized to None @@ -283,43 +286,41 @@ async fn main() -> Result<(), Box> { if !event.is_initialized() { log::warn!("Received udev event with uninitialized device."); } + + let node = match event.devnode() { + None => { continue; }, + Some(node) => { + match node.to_str() { + None => { continue; }, + Some(node) => node, + } + }, + }; + match event.event_type() { EventType::Add => { - if let Some(devnode) = event.devnode() { - let mut device = match Device::open(devnode) { - Err(e) => { - log::error!("Could not open evdev device at {}: {}", devnode.to_string_lossy(), e); - continue; - }, - Ok(device) => device - }; - if arg_devices.contains(&device.name().unwrap_or("")) || check_device_is_keyboard(&device) { - log::info!("Device '{}' at '{}' added.", &device.name().unwrap_or(""), devnode.to_string_lossy()); - let _ = device.grab(); - keyboard_states.push(KeyboardState::new()); - keyboard_stream_map.insert(keyboard_states.len() - 1, device.into_event_stream()?); - } + let mut device = match Device::open(node) { + Err(e) => { + log::error!("Could not open evdev device at {}: {}", node, e); + continue; + }, + Ok(device) => device + }; + let name = device.name().unwrap_or("[unknown]"); + if arg_devices.contains(&name) || check_device_is_keyboard(&device) { + log::info!("Device '{}' at '{}' added.", name, node); + let _ = device.grab(); + keyboard_states.insert(node.to_string(), KeyboardState::new()); + keyboard_stream_map.insert(node.to_string(), device.into_event_stream()?); } } EventType::Remove => { - let udev_physical_path = event.property_value("PHYS") - .and_then(|os_str| os_str.to_str()) - .map(|s| s.replace('\"', "")); - keyboard_stream_map - .iter() - .filter(|(_, stream)| stream.device().physical_path() == udev_physical_path.as_deref()) - .map(|(key, _)| *key) - // Collect all to not remove values while iterating - .collect::>() - .into_iter() - // Reverse to avoid dealing with changing indices for the keyboard_states vector - .rev() - .for_each(|key| { - keyboard_states.remove(key); - if let Some(stream) = keyboard_stream_map.remove(&key) { - log::info!("Device '{}' removed", stream.device().name().unwrap_or("")); - } - }); + if keyboard_stream_map.contains_key(node) { + keyboard_states.remove(node); + let stream = keyboard_stream_map.remove(node).expect("device not in stream_map"); + let name = stream.device().name().unwrap_or("[unknown]"); + log::info!("Device '{}' at '{}' removed", name, node); + } } _ => { log::trace!("Ignored udev event of type: {:?}", event.event_type()); @@ -327,8 +328,8 @@ async fn main() -> Result<(), Box> { } } - Some((i, Ok(event))) = keyboard_stream_map.next() => { - let keyboard_state = &mut keyboard_states[i]; + Some((node, Ok(event))) = keyboard_stream_map.next() => { + let keyboard_state = &mut keyboard_states.get_mut(&node).expect("device not in states map"); let key = match event.kind() { InputEventKind::Key(keycode) => keycode,