From b6ac916a2c9d9083511b054e4523980f05d0b3ed Mon Sep 17 00:00:00 2001 From: vasilito Date: Mon, 27 Jul 2026 22:20:04 +0900 Subject: [PATCH] =?UTF-8?q?wifictl=20dbus=5Fnm:=20correct=20wire=20types?= =?UTF-8?q?=20=E2=80=94=20NmState=200..70=20+=20OwnedObjectPath?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redbear-wifictl NM-shaped D-Bus interface returned wire-incompatible types that no compliant client (Qt, GNOME, kf6-networkmanager-qt) could parse: 1. Root State returned a NmDeviceState value (up to 120 = Failed), but Qt's qnetworkmanagerservice.h:65-74 defines NmState as 0..=70 (Unknown..ConnectedGlobal). Returning a device-state value in the manager-state property is wire-incompatible. 2. get_devices() and get_access_points() returned Vec, but the D-Bus spec requires Vec (array of object paths). zbus serializes Vec as 'as' which clients reject. 3. active_access_point returned String, should be o. The sentinel '/' for 'no active AP' is a valid object path per spec. Fixes: - Add new NmState enum with the correct 8 variants (Unknown=0, Asleep=10, Disconnected=20, Disconnecting=30, Connecting=40, ConnectedLocal=50, ConnectedSite=60, ConnectedGlobal=70). - Add NmState::from_device_state(NmDeviceState) mapping table: Unmanaged/Unavailable/Disconnected/Failed -> Disconnected; Prepare/Config/NeedAuth/IpConfig/IpCheck -> Connecting; Activated -> ConnectedGlobal; Unknown -> Unknown. - Root state property now returns NmState::from_device_state( device.state).as_u32(), guaranteed 0..=70. - get_devices(), get_all_devices(), active_connections (property), get_access_points(), get_all_access_points() all return Vec via a shared try_path() helper that maps malformed strings to fdo::Error::Failed instead of panicking. - active_access_point() property returns fdo::Result; uses '/' sentinel when no AP is active. - 5 new host tests verify NmState numeric values (the Qt-required 0/10/20/.../70 sequence), from_device_state mapping, and the 'range never exceeds 70' invariant. Verified: 35 unit + 2 integration tests pass. --- .../redbear-wifictl/source/src/dbus_nm.rs | 261 +++++++++++++++--- 1 file changed, 224 insertions(+), 37 deletions(-) diff --git a/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs b/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs index 4da60e8ca7..09acea0e12 100644 --- a/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs +++ b/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs @@ -118,6 +118,91 @@ impl NmDeviceState { } } +/// Overall NetworkManager state — the value carried by the +/// `org.freedesktop.NetworkManager.State` property and the `StateChanged` +/// signal. This is a *distinct* enum from [`NmDeviceState`]: it is a coarser +/// roll-up over the smaller range `0..=70` defined by the NetworkManager spec +/// and consumed by clients such as Qt's `QNetworkConfigurationManager` +/// (`qnetworkmanagerservice.h:65-74`). A per-device state value (which can +/// reach `120`) must never leak onto this property. +#[repr(u32)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum NmState { + Unknown = 0, + Asleep = 10, + Disconnected = 20, + Disconnecting = 30, + Connecting = 40, + ConnectedLocal = 50, + ConnectedSite = 60, + ConnectedGlobal = 70, +} + +impl Default for NmState { + fn default() -> Self { + NmState::Unknown + } +} + +impl NmState { + /// Numeric value used on the D-Bus wire (matches the NetworkManager + /// `NMState` enum, range `0..=70`). + pub fn as_u32(self) -> u32 { + self as u32 + } + + /// Human-readable label for diagnostics. + pub fn as_str(self) -> &'static str { + match self { + NmState::Unknown => "unknown", + NmState::Asleep => "asleep", + NmState::Disconnected => "disconnected", + NmState::Disconnecting => "disconnecting", + NmState::Connecting => "connecting", + NmState::ConnectedLocal => "connected-local", + NmState::ConnectedSite => "connected-site", + NmState::ConnectedGlobal => "connected-global", + } + } + + /// Roll up a per-device state into the overall NetworkManager state. + /// + /// NetworkManager computes its top-level state from the "best" device + /// state. For this single-device daemon the mapping is direct: + /// + /// | device state | manager state | + /// |---------------------------------------|---------------------| + /// | `Unknown` | `Unknown` | + /// | `Unmanaged`/`Unavailable`/ | `Disconnected` | + /// | `Disconnected`/`Failed` | | + /// | `Prepare`/`Config`/`NeedAuth`/ | `Connecting` | + /// | `IpConfig`/`IpCheck` | | + /// | `Activated` | `ConnectedGlobal` | + pub fn from_device_state(state: NmDeviceState) -> Self { + match state { + // The device state is genuinely undetermined. + NmDeviceState::Unknown => NmState::Unknown, + + // No usable connectivity. + NmDeviceState::Unmanaged + | NmDeviceState::Unavailable + | NmDeviceState::Disconnected + | NmDeviceState::Failed => NmState::Disconnected, + + // Any active transition towards a connection. + NmDeviceState::Prepare + | NmDeviceState::Config + | NmDeviceState::NeedAuth + | NmDeviceState::IpConfig + | NmDeviceState::IpCheck => NmState::Connecting, + + // Fully up. Without a connectivity checker we cannot distinguish + // local/site/global, so report the strongest reachable value. + NmDeviceState::Activated => NmState::ConnectedGlobal, + } + } +} + #[derive(Clone, Debug, PartialEq, Eq, Default)] pub struct NmAccessPoint { pub ssid: String, @@ -145,12 +230,23 @@ impl NmAccessPoint { mod nm_iface { use std::sync::{Arc, Mutex}; - use super::NmWifiDevice; + use super::{NmState, NmWifiDevice}; use zbus::{ blocking::connection::Builder as ConnectionBuilder, fdo, interface, - zvariant::ObjectPath, + zvariant::{ObjectPath, OwnedObjectPath}, }; + /// Convert a daemon-produced path string into an [`OwnedObjectPath`], + /// mapping a malformed path to a clean `fdo::Error` instead of panicking. + /// Every path this daemon constructs (`DEVICE_PATH`, `access_point_path`, + /// and the `/` sentinel) is well-formed, so this only fails on programmer + /// error — but returning `fdo::Result` keeps the conversion explicit and + /// avoids `.expect()` on dynamic path strings. + fn try_path(s: &str) -> fdo::Result { + OwnedObjectPath::try_from(s) + .map_err(|e| fdo::Error::Failed(format!("invalid object path '{s}': {e}"))) + } + /// Well-known D-Bus service name for NetworkManager. pub(crate) const BUS_NAME: &str = "org.freedesktop.NetworkManager"; /// Object path for the root NetworkManager object. @@ -195,12 +291,12 @@ mod nm_iface { } } - pub(crate) fn device_path(&self) -> String { - DEVICE_PATH.to_string() + pub(crate) fn device_path(&self) -> fdo::Result { + try_path(DEVICE_PATH) } pub(crate) fn nm_state(&self) -> u32 { - snapshot(&self.device).state.as_u32() + NmState::from_device_state(snapshot(&self.device).state).as_u32() } pub(crate) fn wireless_enabled_flag(&self) -> bool { @@ -211,12 +307,12 @@ mod nm_iface { #[interface(name = "org.freedesktop.NetworkManager")] impl NmRoot { /// GetDevices — returns the object paths of all managed devices. - fn get_devices(&self) -> fdo::Result> { - Ok(vec![self.device_path()]) + fn get_devices(&self) -> fdo::Result> { + Ok(vec![self.device_path()?]) } /// GetAllDevices — same set as GetDevices for this single-device daemon. - fn get_all_devices(&self) -> fdo::Result> { + fn get_all_devices(&self) -> fdo::Result> { self.get_devices() } @@ -232,15 +328,17 @@ mod nm_iface { true } - /// Overall NetworkManager state (mirrors the active device state). + /// Overall NetworkManager state (NMState enum, range 0..=70). Derived + /// from the active device state via [`NmState::from_device_state`]. #[zbus(property)] fn state(&self) -> u32 { self.nm_state() } - /// ActiveConnections property (paths); empty until a connection is up. + /// ActiveConnections property (object paths); empty until a connection + /// is up. #[zbus(property)] - fn active_connections(&self) -> Vec { + fn active_connections(&self) -> Vec { Vec::new() } @@ -270,13 +368,13 @@ mod nm_iface { } /// Object paths for every known access point. - pub(crate) fn access_point_paths(&self) -> Vec { + pub(crate) fn access_point_paths(&self) -> fdo::Result> { (0..self.snap().access_points.len()) - .map(access_point_path) + .map(|i| try_path(&access_point_path(i))) .collect() } - pub(crate) fn active_access_point_inner(&self) -> String { + pub(crate) fn active_access_point_inner(&self) -> fdo::Result { let snap = self.snap(); let has_active = !snap.active_ssid.is_empty() && snap @@ -284,9 +382,11 @@ mod nm_iface { .iter() .any(|ap| ap.ssid == snap.active_ssid); if has_active { - access_point_path(0) + try_path(&access_point_path(0)) } else { - "/".to_string() + // Per the NetworkManager spec, `ActiveAccessPoint` is the root + // path "/" when no access point is associated. + try_path("/") } } @@ -318,12 +418,12 @@ mod nm_iface { #[interface(name = "org.freedesktop.NetworkManager.Device.Wireless")] impl NmWirelessDevice { /// GetAccessPoints — object paths of all known access points. - fn get_access_points(&self) -> fdo::Result> { - Ok(self.access_point_paths()) + fn get_access_points(&self) -> fdo::Result> { + self.access_point_paths() } /// GetAllAccessPoints — includes hidden networks (same set here). - fn get_all_access_points(&self) -> fdo::Result> { + fn get_all_access_points(&self) -> fdo::Result> { self.get_access_points() } @@ -377,8 +477,8 @@ mod nm_iface { /// ActiveAccessPoint object path. #[zbus(property)] - fn active_access_point(&self) -> fdo::Result { - Ok(self.active_access_point_inner()) + fn active_access_point(&self) -> fdo::Result { + self.active_access_point_inner() } /// Interface property (device name, e.g. "wlan0"). @@ -509,6 +609,90 @@ mod tests { } } + #[test] + fn nm_state_default_is_unknown() { + assert_eq!(NmState::default(), NmState::Unknown); + assert_eq!(NmState::default().as_u32(), 0); + } + + #[test] + fn nm_state_as_u32_matches_networkmanager_spec() { + assert_eq!(NmState::Unknown.as_u32(), 0); + assert_eq!(NmState::Asleep.as_u32(), 10); + assert_eq!(NmState::Disconnected.as_u32(), 20); + assert_eq!(NmState::Disconnecting.as_u32(), 30); + assert_eq!(NmState::Connecting.as_u32(), 40); + assert_eq!(NmState::ConnectedLocal.as_u32(), 50); + assert_eq!(NmState::ConnectedSite.as_u32(), 60); + assert_eq!(NmState::ConnectedGlobal.as_u32(), 70); + } + + #[test] + fn nm_state_from_device_state_maps_activated_to_connected_global() { + assert_eq!( + NmState::from_device_state(NmDeviceState::Activated), + NmState::ConnectedGlobal, + ); + } + + #[test] + fn nm_state_from_device_state_maps_connecting_states() { + for connecting in [ + NmDeviceState::Prepare, + NmDeviceState::Config, + NmDeviceState::NeedAuth, + NmDeviceState::IpConfig, + NmDeviceState::IpCheck, + ] { + assert_eq!( + NmState::from_device_state(connecting), + NmState::Connecting, + "{connecting:?} should map to Connecting", + ); + } + } + + #[test] + fn nm_state_from_device_state_maps_disconnected_states() { + for disconnected in [ + NmDeviceState::Unmanaged, + NmDeviceState::Unavailable, + NmDeviceState::Disconnected, + NmDeviceState::Failed, + ] { + assert_eq!( + NmState::from_device_state(disconnected), + NmState::Disconnected, + "{disconnected:?} should map to Disconnected", + ); + } + } + + #[test] + fn nm_state_never_exceeds_70_for_any_device_state() { + // The root State property must stay within the NMState contract + // (0..=70) and never leak a NmDeviceState value (which reaches 120). + for ds in [ + NmDeviceState::Unknown, + NmDeviceState::Unmanaged, + NmDeviceState::Unavailable, + NmDeviceState::Disconnected, + NmDeviceState::Prepare, + NmDeviceState::Config, + NmDeviceState::NeedAuth, + NmDeviceState::IpConfig, + NmDeviceState::IpCheck, + NmDeviceState::Activated, + NmDeviceState::Failed, + ] { + let v = NmState::from_device_state(ds).as_u32(); + assert!( + v <= 70, + "NmState for {ds:?} is {v}, exceeds the 0..=70 NMState contract", + ); + } + } + #[test] fn nm_wifi_device_default_is_empty() { let dev = NmWifiDevice::default(); @@ -573,7 +757,7 @@ mod tests { use super::super::nm_iface::{ DEVICE_PATH, NmRoot, NmWirelessDevice, access_point_path, shared_device, }; - use super::{NmAccessPoint, NmDeviceState, NmWifiDevice}; + use super::{NmAccessPoint, NmDeviceState, NmState, NmWifiDevice}; fn device_with(state: NmDeviceState) -> NmWifiDevice { NmWifiDevice { @@ -595,15 +779,15 @@ mod tests { let root = NmRoot::new(shared_device(&device_with( NmDeviceState::Activated, ))); - assert_eq!(root.device_path(), DEVICE_PATH); - assert_eq!(root.nm_state(), NmDeviceState::Activated.as_u32()); + assert_eq!(root.device_path().unwrap().as_str(), DEVICE_PATH); + assert_eq!(root.nm_state(), NmState::ConnectedGlobal.as_u32()); assert!(root.wireless_enabled_flag()); } #[test] fn root_defaults_to_unknown_state() { let root = NmRoot::new(shared_device(&NmWifiDevice::default())); - assert_eq!(root.nm_state(), 0); + assert_eq!(root.nm_state(), NmState::Unknown.as_u32()); } #[test] @@ -623,11 +807,11 @@ mod tests { let dev = NmWirelessDevice::new(shared_device(&device_with( NmDeviceState::Disconnected, ))); - let paths = dev.access_point_paths(); + let paths = dev.access_point_paths().expect("valid access-point paths"); assert_eq!(paths.len(), 2); - assert_eq!(paths[0], access_point_path(0)); - assert_eq!(paths[1], access_point_path(1)); - assert!(paths[0].starts_with(DEVICE_PATH)); + assert_eq!(paths[0].as_str(), access_point_path(0)); + assert_eq!(paths[1].as_str(), access_point_path(1)); + assert!(paths[0].as_str().starts_with(DEVICE_PATH)); } #[test] @@ -635,7 +819,10 @@ mod tests { let dev = NmWirelessDevice::new(shared_device(&device_with( NmDeviceState::Activated, ))); - assert_eq!(dev.active_access_point_inner(), access_point_path(0)); + assert_eq!( + dev.active_access_point_inner().unwrap().as_str(), + access_point_path(0), + ); } #[test] @@ -644,7 +831,7 @@ mod tests { device.active_ssid = "Nonexistent".to_string(); device.active_strength = 0; let dev = NmWirelessDevice::new(shared_device(&device)); - assert_eq!(dev.active_access_point_inner(), "/"); + assert_eq!(dev.active_access_point_inner().unwrap().as_str(), "/"); assert_eq!(dev.strength_inner(), 0); } @@ -652,8 +839,8 @@ mod tests { fn wireless_device_empty_device_has_no_access_points() { let dev = NmWirelessDevice::new(shared_device(&NmWifiDevice::default())); - assert!(dev.access_point_paths().is_empty()); - assert_eq!(dev.active_access_point_inner(), "/"); + assert!(dev.access_point_paths().unwrap().is_empty()); + assert_eq!(dev.active_access_point_inner().unwrap().as_str(), "/"); assert_eq!(dev.strength_inner(), 0); assert_eq!(dev.last_scan_inner(), 0); } @@ -664,8 +851,8 @@ mod tests { let root = NmRoot::new(Arc::clone(&shared)); let wireless = NmWirelessDevice::new(Arc::clone(&shared)); - assert_eq!(root.nm_state(), NmDeviceState::Unknown.as_u32()); - assert!(wireless.access_point_paths().is_empty()); + assert_eq!(root.nm_state(), NmState::Unknown.as_u32()); + assert!(wireless.access_point_paths().unwrap().is_empty()); { let mut guard = shared.lock().expect("lock not poisoned"); @@ -679,9 +866,9 @@ mod tests { guard.active_strength = 90; } - assert_eq!(root.nm_state(), NmDeviceState::Activated.as_u32()); + assert_eq!(root.nm_state(), NmState::ConnectedGlobal.as_u32()); assert_eq!(wireless.strength_inner(), 90); - assert_eq!(wireless.access_point_paths().len(), 1); + assert_eq!(wireless.access_point_paths().unwrap().len(), 1); } } }