From 895946798f7471163dc6b7f23bb2e82df3be5212 Mon Sep 17 00:00:00 2001 From: beviu Date: Tue, 30 Sep 2025 16:00:34 +0200 Subject: [PATCH] Revert "switch-to-configuration-ng: wait for NameOwnerChanged after systemd Reexecute" This reverts commit 67b8817f26366b93f07be8841ae9001b5af4e246. That commit introduces a new error when: - switch-to-configuration-ng spawns the "per-user" switch-to-configuration-ng (the one that does do_user_switch). - That "per-user" switch-to-configuration-ng instance connects to the D-Bus session bus, but the bus is not up yet. - The D-Bus session bus is configured to use systemd socket activation and is currently being started by the systemd user manager. - The D-Bus session bus finishes starting. At this point, only switch-to-configuration-ng is connected to it. Systemd is not connected yet because it waits for a ready notification from the D-Bus session bus. - switch-to-configuration-ng starts listening for a NameOwnerChanged signal with the org.freedesktop.systemd1 bus name. - Systemd is notified that the D-Bus session bus is ready, so it connects to it and acquires the name org.freedesktop.systemd1, which triggers the NameOwnerChanged signal that we listen to. - switch-to-configuration-ng marks systemd as reexecuted, but it's _not started reexecuting yet_. - Then switch-to-configuration-ng tells systemd to reexecute. - The loop waiting for systemd to reexecute [1] is never entered. - switch-to-configuration-ng asks systemd to restart a unit but that fails because systemd is reexecuting and the method call is sent to the old deamon. The idea with #442756 was to: 1. Remove the timeout for the Reexecute method call because it would fail in a non-obvious way when systemd took too much time to execute, and that could happen when there are many .mount units. 2. While we're at it, use the NameOwnerChanged signal to determine when systemd is done reexecuting before we proceed with the other systemd calls, instead of waiting for the Reexecute method call to error. But in fact, it seems like 2. is not a good idea. Waiting for the NameOwnerChanged signal has the disadvantage of not letting us know if it is due to our Reexecute method call or someone else's, or even just systemd that's just connected to the D-Bus in between as is the case here. It might still be a good idea to increase the timeout for the Reexecute method call to fix the original error, but at least let's revert the PR. [1]: https://github.com/NixOS/nixpkgs/blob/ec481667d737b1b366ac55d72ec09d101d842533/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs#L989 --- .../switch-to-configuration-ng/src/build.rs | 4 - .../src/org.freedesktop.DBus.xml | 12 --- .../src/src/main.rs | 81 ++----------------- 3 files changed, 6 insertions(+), 91 deletions(-) delete mode 100644 pkgs/by-name/sw/switch-to-configuration-ng/src/org.freedesktop.DBus.xml diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/build.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/build.rs index bc07ec47a661..41fed3086e69 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/build.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/build.rs @@ -18,10 +18,6 @@ fn main() { let out_path = std::path::PathBuf::from(std::env::var("OUT_DIR").unwrap()); - let fdo_dbus_code = code_for_dbus_xml("org.freedesktop.DBus.xml"); - let mut file = std::fs::File::create(out_path.join("fdo_dbus.rs")).unwrap(); - file.write_all(fdo_dbus_code.as_bytes()).unwrap(); - let systemd_manager_code = code_for_dbus_xml(systemd_dbus_interface_dir.join("org.freedesktop.systemd1.Manager.xml")); let mut file = std::fs::File::create(out_path.join("systemd_manager.rs")).unwrap(); diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/org.freedesktop.DBus.xml b/pkgs/by-name/sw/switch-to-configuration-ng/src/org.freedesktop.DBus.xml deleted file mode 100644 index 6ab33de9dc40..000000000000 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/org.freedesktop.DBus.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - - - - - - diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs index 4e84e8d1ae09..705b9284f6a1 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/src/main.rs @@ -17,8 +17,6 @@ use std::{ use anyhow::{anyhow, bail, Context, Result}; use dbus::{ blocking::{stdintf::org_freedesktop_dbus::Properties, LocalConnection, Proxy}, - channel::Sender, - strings::{BusName, Interface, Member}, Message, }; use glob::glob; @@ -34,15 +32,6 @@ use nix::{ use regex::Regex; use syslog::Facility; -mod fdo_dbus { - #![allow(non_upper_case_globals)] - #![allow(non_camel_case_types)] - #![allow(non_snake_case)] - #![allow(unused)] - #![allow(clippy::all)] - include!(concat!(env!("OUT_DIR"), "/fdo_dbus.rs")); -} - mod systemd_manager { #![allow(non_upper_case_globals)] #![allow(non_camel_case_types)] @@ -61,9 +50,7 @@ mod logind_manager { include!(concat!(env!("OUT_DIR"), "/logind_manager.rs")); } -use crate::{ - fdo_dbus::OrgFreedesktopDBusNameOwnerChanged, systemd_manager::OrgFreedesktopSystemd1Manager, -}; +use crate::systemd_manager::OrgFreedesktopSystemd1Manager; use crate::{ logind_manager::OrgFreedesktopLogin1Manager, systemd_manager::{ @@ -907,14 +894,6 @@ impl std::fmt::Display for Job { } } -fn fdo_dbus_proxy(conn: &LocalConnection) -> Proxy<'_, &LocalConnection> { - conn.with_proxy( - "org.freedesktop.DBus", - "/org/freedesktop/DBus", - Duration::from_millis(500), - ) -} - fn systemd1_proxy(conn: &LocalConnection) -> Proxy<'_, &LocalConnection> { conn.with_proxy( "org.freedesktop.systemd1", @@ -951,54 +930,6 @@ fn remove_file_if_exists(p: impl AsRef) -> std::io::Result<()> { } } -fn reexecute_systemd_manager( - dbus_conn: &LocalConnection, - fdo_dbus: &Proxy<'_, &LocalConnection>, -) -> anyhow::Result<()> { - let reexecute_done = Rc::new(RefCell::new(false)); - let _reexecute_done = reexecute_done.clone(); - let owner_changed_token = fdo_dbus - .match_signal( - move |signal: OrgFreedesktopDBusNameOwnerChanged, _: &LocalConnection, _: &Message| { - if signal.name.as_str() == "org.freedesktop.systemd1" { - *_reexecute_done.borrow_mut() = true; - } - - true - }, - ) - .context("Failed to add signal match for DBus name owner changes")?; - - let bus_name = BusName::from("org.freedesktop.systemd1"); - let object_path = dbus::Path::from("/org/freedesktop/systemd1"); - let interface = Interface::new("org.freedesktop.systemd1.Manager") - .expect("the org.freedesktop.systemd1.Manager interface name should be valid"); - let method_name = Member::new("Reexecute").expect("the Reexecute method name should be valid"); - - // Systemd does not reply to the Reexecute method. - let _serial = dbus_conn - .send(Message::method_call( - &bus_name, - &object_path, - &interface, - &method_name, - )) - .map_err(|_err| anyhow!("Failed to send org.freedesktop.systemd1.Manager.Reexecute"))?; - - log::debug!("waiting for systemd to finish reexecuting"); - while !*reexecute_done.borrow() { - _ = dbus_conn - .process(Duration::from_secs(500)) - .context("Failed to process dbus messages")?; - } - - dbus_conn - .remove_match(owner_changed_token) - .context("Failed to remove jobs token")?; - - Ok(()) -} - /// Performs switch-to-configuration functionality for a single non-root user fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { if Path::new(&parent_exe) @@ -1014,11 +945,8 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { } let dbus_conn = LocalConnection::new_session().context("Failed to open dbus connection")?; - let fdo_dbus = fdo_dbus_proxy(&dbus_conn); let systemd = systemd1_proxy(&dbus_conn); - reexecute_systemd_manager(&dbus_conn, &fdo_dbus)?; - let nixos_activation_done = Rc::new(RefCell::new(false)); let _nixos_activation_done = nixos_activation_done.clone(); let jobs_token = systemd @@ -1035,6 +963,10 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { ) .context("Failed to add signal match for systemd removed jobs")?; + // The systemd user session seems to not send a Reloaded signal, so we don't have anything to + // wait on here. + _ = systemd.reexecute(); + systemd .restart_unit("nixos-activation.service", "replace") .context("Failed to restart nixos-activation.service")?; @@ -1207,7 +1139,6 @@ won't take effect until you reboot the system. let mut units_to_reload = map_from_list_file(RELOAD_LIST_FILE); let dbus_conn = LocalConnection::new_system().context("Failed to open dbus connection")?; - let fdo_dbus = fdo_dbus_proxy(&dbus_conn); let systemd = systemd1_proxy(&dbus_conn); let logind = login1_proxy(&dbus_conn); @@ -1733,7 +1664,7 @@ won't take effect until you reboot the system. // just in case the new one has trouble communicating with the running pid 1. if restart_systemd { eprintln!("restarting systemd..."); - reexecute_systemd_manager(&dbus_conn, &fdo_dbus)?; + _ = systemd.reexecute(); // we don't get a dbus reply here log::debug!("waiting for systemd restart to finish"); while !*systemd_reload_status.borrow() {