fix(networkmanager): update wifi state on strength changes

Also fix some bugs with the overall device state change listeners.

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<PathMap<'l, ActiveConnectionDbusProxyBlocking<'l>>>,
     devices: RwLock<PathMap<'l, DeviceDbusProxyBlocking<'l>>>,
+    access_point: RwLock<Option<(ObjectPath<'l>, 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<ClientInner<'static>>) -> 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<Client> {
@@ -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)
+                    let changes = {
-                        .get(&path)
+                        let path_map = read_lock!(client.$containing_list);
-                        .expect("Should contain the key upon watcher start")
+                        let Some(device) = path_map.get(&path) else {
-                        .$property_changes();
+                            // 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!(
+        // initialize active_connections proxys
-            self.0,
+        {
-            active_connections,
+            let active_connections = HashMap::new();
-            ActiveConnectionDbusProxyBlocking
+            for active_connection_path in self.0.root_object.active_connections()? {
-        );
+                let proxy = ActiveConnectionDbusProxyBlocking::builder(&self.0.dbus_connection)
-        initialise_path_map!(self.0, devices, DeviceDbusProxyBlocking, |path| {
+                    .path(active_connection_path.clone())?
-            spawn_property_watcher!(self.0, path, receive_state_changed, devices, |client| {
+                    .build()?;
-                update_state_for_device_change!(client);
+                self.0
-            });
+                    .active_connections
-        });
+                    .write()
-        self.0.state.set(State {
+                    .unwrap()
-            wired: determine_wired_state(&read_lock!(self.0.devices))?,
+                    .insert(active_connection_path, proxy);
-            wifi: determine_wifi_state(&self.0.dbus_connection, &read_lock!(self.0.devices))?,
+            }
-            cellular: determine_cellular_state(&read_lock!(self.0.devices))?,
+            *write_lock!(self.0.active_connections) = active_connections;
-            vpn: determine_vpn_state(&read_lock!(self.0.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();
                 });
             }
         );

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(
+pub(super) fn determine_wifi_state(client: &super::Client) -> Result<WifiState> {
-    dbus_connection: &zbus::blocking::Connection,
+    let dbus_connection = &client.0.dbus_connection;
-    devices: &PathMap<DeviceDbusProxyBlocking>,
+    let devices = &read_lock!(client.0.devices);
-) -> Result<WifiState> {
+    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
+        // Pick the first address. Not sure if there are cases when there are more than one
-        // (at least for wifi).
+        // address.
+        // These could fail if the access point concurrently changes.
         let address = &address_data
-            .iter()
+            .first()
-            .next()
             .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)