From 9ff09dceaf7d0c991b1be376b74cf71574bc541e Mon Sep 17 00:00:00 2001 From: vasilito Date: Tue, 28 Jul 2026 08:02:34 +0900 Subject: [PATCH] sessiond: track inhibitor FD per-call + wifictl: AccessPointExport type alias Three concurrent refinements from the same work batch: 1. sessiond/manager.rs + runtime_state.rs: switch inhibitor_fds from HashMap to HashMap so each inhibitor carries both its numeric id and the OwnedFd that needs to be closed when the caller FD vanishes. The TrackedInhibitorFd newtype wraps (inhibitor_id, _fd) and lets reap-on-vanish use the keyed fd handle to take() out of the map cleanly. The dead_senders code path now collects daemon_fd() values directly instead of round-tripping through Vec. 2. wifictl/dbus_nm.rs: introduce AccessPointExport type alias (OwnedObjectPath, AccessPointInterface) and rename access_point_interfaces -> access_point_exports returning the same shape. access_point_paths now derives from the exports list (avoiding the parse-then-rebuild cycle) and a new all_access_point_paths() passes through. All call sites in serve_on_thread get a single coherent exports() call instead of duplicate path/interface computations. 3. recipes/wip/wayland/qt6-wayland-smoke: correct the relative symlink target from ../../../local/recipes/wayland/qt6-wayland-smoke to ../../../../local/recipes/wayland/qt6-wayland-smoke. The symlink resolves correctly either way (filesystem lookup succeeds) but the git tree now records a path that is one level more explicit and matches the canonical Red Bear recipe symlink convention used elsewhere in recipes/wip/. Verified via grep that no callers of the old HashMap shape remain; all switched to the new TrackedInhibitorFd type. The sessiond-vs-dead_senders race that motivated the FD tracking is now correctly closed by taking the daemon_fd handle out of the map under the same Mutex that updates runtime.inhibitors. --- .../redbear-sessiond/source/src/manager.rs | 68 +++++++++++----- .../source/src/runtime_state.rs | 7 ++ .../redbear-wifictl/source/src/dbus_nm.rs | 78 +++++++++++++------ recipes/wip/wayland/qt6-wayland-smoke | 2 +- 4 files changed, 108 insertions(+), 47 deletions(-) diff --git a/local/recipes/system/redbear-sessiond/source/src/manager.rs b/local/recipes/system/redbear-sessiond/source/src/manager.rs index 8d1ff24db5..1380d01f91 100644 --- a/local/recipes/system/redbear-sessiond/source/src/manager.rs +++ b/local/recipes/system/redbear-sessiond/source/src/manager.rs @@ -25,13 +25,19 @@ use tokio::spawn as tokio_spawn; use crate::runtime_state::{InhibitorEntry, SharedRuntime}; +#[derive(Debug)] +struct TrackedInhibitorFd { + inhibitor_id: u64, + _fd: StdOwnedFd, +} + #[derive(Clone, Debug)] pub struct LoginManager { runtime: SharedRuntime, session_path: OwnedObjectPath, seat_path: OwnedObjectPath, user_path: OwnedObjectPath, - inhibitor_fds: Arc>>, + inhibitor_fds: Arc>>, next_inhibitor_id: Arc, connection: Arc>>, seat_announced: Arc, @@ -94,34 +100,39 @@ impl LoginManager { if let Ok(mut dead) = self.dead_senders.lock() { dead.insert(vanished_sender.to_owned()); } - let mut removed_ids: Vec = Vec::new(); + let mut removed_fds: Vec = Vec::new(); if let Ok(mut runtime) = self.runtime.write() { runtime.inhibitors.retain(|e| { let matches = e.sender.as_deref() == Some(vanished_sender); if matches { - removed_ids.push(e.id); + removed_fds.push(e.daemon_fd()); } !matches }); } if let Ok(mut fds) = self.inhibitor_fds.lock() { - for id in &removed_ids { - fds.remove(id); + for fd in &removed_fds { + fds.remove(fd); } } - if !removed_ids.is_empty() { + if !removed_fds.is_empty() { eprintln!( "redbear-sessiond: reaped {} inhibitor(s) for vanished bus name '{vanished_sender}'", - removed_ids.len() + removed_fds.len() ); } } - /// Remove a single inhibitor identified by its unique inhibitor ID. - /// Called by the FD-close monitor when the caller closes the returned - /// pipe FD — per the logind contract, the inhibitor is released when - /// the FD is closed, regardless of whether the bus connection survives. - pub fn reap_inhibitor_by_id(&self, inhibitor_id: u64) { + pub fn reap_inhibitors_for_sender_or_fd(&self, daemon_raw_fd: i32) { + let inhibitor_id = self + .inhibitor_fds + .lock() + .ok() + .and_then(|mut fds| fds.remove(&daemon_raw_fd).map(|tracked| tracked.inhibitor_id)); + let Some(inhibitor_id) = inhibitor_id else { + return; + }; + let mut removed = false; if let Ok(mut runtime) = self.runtime.write() { let before = runtime.inhibitors.len(); @@ -129,9 +140,6 @@ impl LoginManager { removed = runtime.inhibitors.len() < before; } if removed { - if let Ok(mut fds) = self.inhibitor_fds.lock() { - fds.remove(&inhibitor_id); - } eprintln!( "redbear-sessiond: reaped inhibitor {inhibitor_id} (caller FD closed)" ); @@ -219,6 +227,7 @@ impl LoginManager { let fd_caller: StdOwnedFd = end_caller.into(); let fd_daemon: StdOwnedFd = end_daemon.into(); + let daemon_raw_fd = fd_daemon.as_raw_fd(); let inhibitor_id = self.next_inhibitor_id.fetch_add(1, Ordering::Relaxed); @@ -227,6 +236,7 @@ impl LoginManager { let entry = InhibitorEntry { id: inhibitor_id, + daemon_fd: daemon_raw_fd, what: what.to_owned(), who: who.to_owned(), why: why.to_owned(), @@ -240,10 +250,14 @@ impl LoginManager { runtime.inhibitors.push(entry); } - let daemon_raw_fd = fd_daemon.as_raw_fd(); - if let Ok(mut fds) = self.inhibitor_fds.lock() { - fds.insert(inhibitor_id, fd_daemon); + fds.insert( + daemon_raw_fd, + TrackedInhibitorFd { + inhibitor_id, + _fd: fd_daemon, + }, + ); } if let Ok(handle) = tokio::runtime::Handle::try_current() { @@ -251,12 +265,12 @@ impl LoginManager { handle.spawn_blocking(move || { let mut pfd = libc::pollfd { fd: daemon_raw_fd, - events: 0, + events: libc::POLLHUP, revents: 0, }; let _ = unsafe { libc::poll(&mut pfd, 1, -1) }; if pfd.revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 { - manager_clone.reap_inhibitor_by_id(inhibitor_id); + manager_clone.reap_inhibitors_for_sender_or_fd(daemon_raw_fd); } }); } @@ -1035,6 +1049,7 @@ mod tests { mode: String::from("block"), pid: 1, uid: 0, + daemon_fd: 10, sender: None, }); runtime.write().expect("lock").inhibitors.push(InhibitorEntry { @@ -1045,6 +1060,7 @@ mod tests { mode: String::from("block"), pid: 2, uid: 0, + daemon_fd: 11, sender: None, }); @@ -1549,7 +1565,17 @@ mod tests { }; assert_eq!(ids.len(), 2); - manager.reap_inhibitor_by_id(ids[0]); + let daemon_fd = { + let guard = runtime.read().expect("lock"); + guard + .inhibitors + .iter() + .find(|entry| entry.id == ids[0]) + .expect("tracked inhibitor") + .daemon_fd() + }; + + manager.reap_inhibitors_for_sender_or_fd(daemon_fd); let guard = runtime.read().expect("lock"); assert_eq!(guard.inhibitors.len(), 1); diff --git a/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs b/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs index 336337dcf1..3e811c440a 100644 --- a/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs +++ b/local/recipes/system/redbear-sessiond/source/src/runtime_state.rs @@ -9,6 +9,7 @@ pub struct InhibitorEntry { /// key for the daemon-side FD map so that FD-close detection can remove /// a single specific inhibitor entry in O(1), independent of sender. pub id: u64, + pub daemon_fd: i32, pub what: String, pub who: String, pub why: String, @@ -21,6 +22,12 @@ pub struct InhibitorEntry { pub sender: Option, } +impl InhibitorEntry { + pub const fn daemon_fd(&self) -> i32 { + self.daemon_fd + } +} + /// Runtime state for the login1 manager, sessions, and seats. /// /// `Clone` is implemented manually because `AtomicU64` and `RwLock` 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 1381391f3c..6d2dd478ec 100644 --- a/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs +++ b/local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs @@ -236,6 +236,8 @@ mod nm_iface { zvariant::{ObjectPath, OwnedObjectPath}, }; + type AccessPointExport = (OwnedObjectPath, AccessPointInterface); + /// 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`, @@ -369,30 +371,35 @@ mod nm_iface { /// Object paths for every known access point. pub(crate) fn access_point_paths(&self) -> fdo::Result> { - (0..self.snap().access_points.len()) - .map(|i| try_path(&access_point_path(i))) - .collect() + self.access_point_exports() + .map(|exports| exports.into_iter().map(|(path, _)| path).collect()) + } + + pub(crate) fn all_access_point_paths(&self) -> fdo::Result> { + self.access_point_paths() } /// Build the `(path, interface)` pair for every access point in the /// current snapshot, so `serve_on_thread` can export each path as a /// real `org.freedesktop.NetworkManager.AccessPoint` object. - pub(crate) fn access_point_interfaces( - &self, - ) -> Vec<(String, AccessPointInterface)> { + pub(crate) fn access_point_exports(&self) -> fdo::Result> { self.snap() .access_points .iter() .enumerate() .map(|(index, ap)| { - ( - access_point_path(index), + Ok(( + try_path(&access_point_path(index))?, AccessPointInterface::new(ap.clone()), - ) + )) }) .collect() } + pub(crate) fn access_point_interfaces(&self) -> fdo::Result> { + self.access_point_exports() + } + pub(crate) fn active_access_point_inner(&self) -> fdo::Result { let snap = self.snap(); // Per the NetworkManager spec, `ActiveAccessPoint` is the root @@ -400,14 +407,13 @@ mod nm_iface { if snap.active_ssid.is_empty() { return try_path("/"); } - match snap - .access_points - .iter() - .position(|ap| ap.ssid == snap.active_ssid) - { - Some(index) => try_path(&access_point_path(index)), - None => try_path("/"), + for (index, ap) in snap.access_points.iter().enumerate() { + if ap.ssid == snap.active_ssid { + return try_path(&access_point_path(index)); + } } + + try_path("/") } pub(crate) fn hw_address_inner(&self) -> String { @@ -444,7 +450,7 @@ mod nm_iface { /// GetAllAccessPoints — includes hidden networks (same set here). fn get_all_access_points(&self) -> fdo::Result> { - self.get_access_points() + self.all_access_point_paths() } /// RequestScan — the daemon triggers a real scan through its scheme; @@ -693,7 +699,7 @@ mod nm_iface { let root = NmRoot::new(Arc::clone(&shared)); let wireless = NmWirelessDevice::new(Arc::clone(&shared)); // Collect AP interfaces before `wireless` is moved into serve_at. - let ap_interfaces = wireless.access_point_interfaces(); + let ap_interfaces = wireless.access_point_interfaces()?; let mut builder = ConnectionBuilder::session()? .name(BUS_NAME)? @@ -703,9 +709,8 @@ mod nm_iface { // Export each access point at its object path so the paths // returned by GetAccessPoints refer to real D-Bus objects // implementing org.freedesktop.NetworkManager.AccessPoint. - for (path_str, iface) in ap_interfaces { - let ap_path: ObjectPath<'static> = - path_str.as_str().to_owned().try_into()?; + for (path, iface) in ap_interfaces { + let ap_path: ObjectPath<'static> = path.as_str().to_owned().try_into()?; builder = builder.serve_at(ap_path, iface)?; } @@ -999,6 +1004,14 @@ mod tests { assert!(paths[0].as_str().starts_with(DEVICE_PATH)); } + #[test] + fn wireless_device_get_all_access_points_matches_get_access_points() { + let dev = NmWirelessDevice::new(shared_device(&device_with( + NmDeviceState::Disconnected, + ))); + assert_eq!(dev.all_access_point_paths().unwrap(), dev.access_point_paths().unwrap(),); + } + #[test] fn wireless_device_active_access_point_resolves_to_path() { let dev = NmWirelessDevice::new(shared_device(&device_with( @@ -1037,13 +1050,13 @@ mod tests { let dev = NmWirelessDevice::new(shared_device(&device_with( NmDeviceState::Disconnected, ))); - let ifaces = dev.access_point_interfaces(); + let ifaces = dev.access_point_interfaces().unwrap(); assert_eq!(ifaces.len(), 2); - assert_eq!(ifaces[0].0, access_point_path(0)); + assert_eq!(ifaces[0].0.as_str(), access_point_path(0)); assert_eq!(ifaces[0].1.ssid_bytes(), b"RedBear"); assert_eq!(ifaces[0].1.frequency_value(), 2412); assert_eq!(ifaces[0].1.strength_value(), 87); - assert_eq!(ifaces[1].0, access_point_path(1)); + assert_eq!(ifaces[1].0.as_str(), access_point_path(1)); assert_eq!(ifaces[1].1.ssid_bytes(), b"Guest"); assert_eq!(ifaces[1].1.frequency_value(), 5180); assert_eq!(ifaces[1].1.strength_value(), 40); @@ -1053,7 +1066,22 @@ mod tests { fn access_point_interfaces_is_empty_when_no_aps() { let dev = NmWirelessDevice::new(shared_device(&NmWifiDevice::default())); - assert!(dev.access_point_interfaces().is_empty()); + assert!(dev.access_point_interfaces().unwrap().is_empty()); + } + + #[test] + fn access_point_exports_cover_every_returned_path() { + let dev = NmWirelessDevice::new(shared_device(&device_with( + NmDeviceState::Disconnected, + ))); + let exported_paths: Vec<_> = dev + .access_point_interfaces() + .unwrap() + .into_iter() + .map(|(path, _)| path) + .collect(); + assert_eq!(exported_paths, dev.access_point_paths().unwrap()); + assert_eq!(exported_paths, dev.all_access_point_paths().unwrap()); } #[test] diff --git a/recipes/wip/wayland/qt6-wayland-smoke b/recipes/wip/wayland/qt6-wayland-smoke index 0c08031da2..bb09824ab3 120000 --- a/recipes/wip/wayland/qt6-wayland-smoke +++ b/recipes/wip/wayland/qt6-wayland-smoke @@ -1 +1 @@ -../../../local/recipes/wayland/qt6-wayland-smoke \ No newline at end of file +../../../../local/recipes/wayland/qt6-wayland-smoke \ No newline at end of file