From a34c34c5fc51d7cd473cb64bdcb7467490192391 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sun, 14 Jul 2024 21:51:48 -0300 Subject: [PATCH] fix(networkmanager): update wifi state on strength changes Also fix some bugs with the overall device state change listeners. --- src/clients/networkmanager/mod.rs | 158 ++++++++++++++++------------ src/clients/networkmanager/state.rs | 59 +++++++++-- 2 files changed, 139 insertions(+), 78 deletions(-) diff --git a/src/clients/networkmanager/mod.rs b/src/clients/networkmanager/mod.rs index 57bed1b..1134cf0 100644 --- a/src/clients/networkmanager/mod.rs +++ b/src/clients/networkmanager/mod.rs @@ -8,7 +8,8 @@ use zbus::blocking::Connection; use zbus::zvariant::ObjectPath; use crate::clients::networkmanager::dbus::{ - ActiveConnectionDbusProxyBlocking, DbusProxyBlocking, DeviceDbusProxyBlocking, + AccessPointDbusProxyBlocking, ActiveConnectionDbusProxyBlocking, DbusProxyBlocking, + DeviceDbusProxyBlocking, }; use crate::clients::networkmanager::state::{ determine_cellular_state, determine_vpn_state, determine_wifi_state, determine_wired_state, @@ -32,8 +33,22 @@ struct ClientInner<'l> { root_object: &'l DbusProxyBlocking<'l>, active_connections: RwLock>>, devices: RwLock>>, + access_point: RwLock, AccessPointDbusProxyBlocking<'l>)>>, dbus_connection: Connection, } +impl ClientInner<'static> { + /// Query the state information for each device. This method can fail at random if the + /// connection changes while querying the information. + fn update_state_for_device_change(self: &Arc>) -> Result<()> { + self.state.set(State { + wired: determine_wired_state(&read_lock!(self.devices))?, + wifi: determine_wifi_state(&Client(self.clone()))?, + cellular: determine_cellular_state(&read_lock!(self.devices))?, + vpn: self.state.get_cloned().vpn, + }); + Ok(()) + } +} impl Client { fn new() -> Result { @@ -55,48 +70,12 @@ impl Client { root_object, active_connections: RwLock::new(HashMap::new()), devices: RwLock::new(HashMap::new()), + access_point: RwLock::new(None), dbus_connection, }))) } fn run(&self) -> Result<()> { - macro_rules! update_state_for_device_change { - ($client:ident) => { - $client.state.set(State { - wired: determine_wired_state(&read_lock!($client.devices))?, - wifi: determine_wifi_state( - &$client.dbus_connection, - &read_lock!($client.devices), - )?, - cellular: determine_cellular_state(&read_lock!($client.devices))?, - vpn: $client.state.get_cloned().vpn, - }); - }; - } - - macro_rules! initialise_path_map { - ( - $client:expr, - $path_map:ident, - $proxy_type:ident - $(, |$new_path:ident| $property_watcher:expr)* - ) => { - let new_paths = $client.root_object.$path_map()?; - let mut path_map = HashMap::new(); - for new_path in new_paths { - let new_proxy = $proxy_type::builder(&$client.dbus_connection) - .path(new_path.clone())? - .build()?; - path_map.insert(new_path.clone(), new_proxy); - $({ - let $new_path = &new_path; - $property_watcher; - })* - } - *write_lock!($client.$path_map) = path_map; - }; - } - macro_rules! spawn_path_list_watcher { ( $client:expr, @@ -111,29 +90,33 @@ impl Client { let changes = client.root_object.$property_changes(); for _ in changes { let mut new_path_map = HashMap::new(); + let new_paths = client.root_object.$property()?; { - let new_paths = client.root_object.$property()?; let path_map = read_lock!(client.$property); - for new_path in new_paths { + for new_path in &new_paths { if path_map.contains_key(&new_path) { let proxy = path_map - .get(&new_path) + .get(new_path) .expect("Should contain the key, guarded by runtime check"); - new_path_map.insert(new_path, proxy.to_owned()); + new_path_map.insert(new_path.clone(), proxy.to_owned()); } else { let new_proxy = $proxy_type::builder(&client.dbus_connection) .path(new_path.clone())? .build()?; new_path_map.insert(new_path.clone(), new_proxy); - $({ - let $property_client = &client; - let $new_path = &new_path; - $property_watcher; - })* } } } *write_lock!(client.$property) = new_path_map; + + for _new_path in &new_paths { + $({ + let $property_client = &client; + let $new_path = _new_path; + $property_watcher; + })* + } + let $state_client = &client; $state_update; } @@ -153,12 +136,18 @@ impl Client { let client = $client.clone(); let path = $path.clone(); spawn_blocking_result!({ - let changes = read_lock!(client.$containing_list) - .get(&path) - .expect("Should contain the key upon watcher start") - .$property_changes(); + let changes = { + let path_map = read_lock!(client.$containing_list); + let Some(device) = path_map.get(&path) else { + // this item could have been removed before the watcher was initialized + tracing::warn!("item removed before first iteration"); + return Ok(()); + }; + device.$property_changes() + }; for _ in changes { if !read_lock!(client.$containing_list).contains_key(&path) { + // this item no longer exits break; } let $inner_client = &client; @@ -169,22 +158,50 @@ impl Client { }; } - initialise_path_map!( - self.0, - active_connections, - ActiveConnectionDbusProxyBlocking - ); - initialise_path_map!(self.0, devices, DeviceDbusProxyBlocking, |path| { - spawn_property_watcher!(self.0, path, receive_state_changed, devices, |client| { - update_state_for_device_change!(client); - }); - }); - self.0.state.set(State { - wired: determine_wired_state(&read_lock!(self.0.devices))?, - wifi: determine_wifi_state(&self.0.dbus_connection, &read_lock!(self.0.devices))?, - cellular: determine_cellular_state(&read_lock!(self.0.devices))?, - vpn: determine_vpn_state(&read_lock!(self.0.active_connections))?, - }); + // initialize active_connections proxys + { + let active_connections = HashMap::new(); + for active_connection_path in self.0.root_object.active_connections()? { + let proxy = ActiveConnectionDbusProxyBlocking::builder(&self.0.dbus_connection) + .path(active_connection_path.clone())? + .build()?; + self.0 + .active_connections + .write() + .unwrap() + .insert(active_connection_path, proxy); + } + *write_lock!(self.0.active_connections) = active_connections; + } + + // initialize devices proxys and watchers + { + let devices = self.0.root_object.devices()?; + let mut path_map = HashMap::new(); + for device_path in &devices { + let proxy = DeviceDbusProxyBlocking::builder(&self.0.dbus_connection) + .path(device_path.clone())? + .build()?; + path_map.insert(device_path.clone(), proxy); + } + *write_lock!((self.0).devices) = path_map; + + tracing::debug!("initialize devices: {:?}", devices); + + for device_path in devices { + spawn_property_watcher!( + self.0, + device_path, + receive_state_changed, + devices, + |client| { + let _ = client.update_state_for_device_change(); + } + ); + } + } + + let _ = self.0.update_state_for_device_change(); spawn_path_list_watcher!( self.0, @@ -192,6 +209,7 @@ impl Client { receive_active_connections_changed, ActiveConnectionDbusProxyBlocking, |client| { + tracing::debug!("active connections changed"); client.state.set(State { wired: client.state.get_cloned().wired, wifi: client.state.get_cloned().wifi, @@ -206,11 +224,13 @@ impl Client { receive_devices_changed, DeviceDbusProxyBlocking, |client| { - update_state_for_device_change!(client); + tracing::debug!("devices changed"); + let _ = client.update_state_for_device_change(); }, |client, path| { spawn_property_watcher!(client, path, receive_state_changed, devices, |client| { - update_state_for_device_change!(client); + tracing::debug!("device state changed"); + let _ = client.update_state_for_device_change(); }); } ); diff --git a/src/clients/networkmanager/state.rs b/src/clients/networkmanager/state.rs index 5cc1532..66635ba 100644 --- a/src/clients/networkmanager/state.rs +++ b/src/clients/networkmanager/state.rs @@ -5,6 +5,7 @@ use crate::clients::networkmanager::dbus::{ DeviceState, DeviceType, DeviceWirelessDbusProxyBlocking, Ip4ConfigDbusProxyBlocking, }; use crate::clients::networkmanager::PathMap; +use crate::{error, read_lock, spawn_blocking, spawn_blocking_result, write_lock}; #[derive(Clone, Debug)] pub struct State { @@ -91,10 +92,11 @@ pub(super) fn determine_wired_state( } } -pub(super) fn determine_wifi_state( - dbus_connection: &zbus::blocking::Connection, - devices: &PathMap, -) -> Result { +pub(super) fn determine_wifi_state(client: &super::Client) -> Result { + let dbus_connection = &client.0.dbus_connection; + let devices = &read_lock!(client.0.devices); + let access_point_ = &client.0.access_point; + let mut present = false; let mut enabled = false; let mut connected = None; @@ -133,11 +135,11 @@ pub(super) fn determine_wifi_state( .path(device.ip4_config()?.clone())? .build()?; let address_data = ip4config.address_data()?; - // pick the first address. not sure if there are cases when there are more than one address - // (at least for wifi). + // Pick the first address. Not sure if there are cases when there are more than one + // address. + // These could fail if the access point concurrently changes. let address = &address_data - .iter() - .next() + .first() .ok_or_else(|| Report::msg("No address in IP4Config"))?; let ip4_address = address .get("address") @@ -146,12 +148,51 @@ pub(super) fn determine_wifi_state( .get("prefix") .ok_or_else(|| Report::msg("IP address data object must have a prefix"))?; + let strength = access_point.strength()?; + + 'block: { + if let Some((ref path, _)) = *read_lock!(access_point_) { + if path == access_point.path() { + break 'block; + } + } + + let access_point_path = access_point.path().to_owned(); + + *write_lock!(*access_point_) = Some((access_point_path.clone(), access_point)); + + let client = client.0.clone(); + spawn_blocking_result!({ + let changes = { + let access_point = read_lock!(client.access_point); + let Some((_, access_point)) = access_point.as_ref() else { + return Ok(()); + }; + access_point.receive_strength_changed() + }; + + for _ in changes { + if read_lock!(client.access_point) + .as_ref() + .map_or(false, |(p, _)| *p != access_point_path) + { + break; + } + + tracing::trace!("accesspoint strength changed"); + client.update_state_for_device_change()?; + } + + Ok(()) + }); + } + Ok(WifiState::Connected(WifiConnectedState { ssid, bssid, ip4_address: String::try_from(ip4_address.to_owned()).unwrap_or_default(), ip4_prefix: u32::try_from(ip4_prefix.to_owned()).unwrap_or_default(), - strength: access_point.strength().unwrap_or(0), + strength, })) } else if enabled { Ok(WifiState::Disconnected)