From 3e8707efe6a91cc8057d6124a3cd26cb669daf86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 11 Aug 2023 12:33:02 +0100 Subject: [PATCH 01/11] Fix the connect() function error --- rust/agama-lib/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/agama-lib/src/lib.rs b/rust/agama-lib/src/lib.rs index c0ba6866e3..a3c055b55a 100644 --- a/rust/agama-lib/src/lib.rs +++ b/rust/agama-lib/src/lib.rs @@ -50,6 +50,6 @@ pub async fn connection_to(address: &str) -> Result Date: Fri, 11 Aug 2023 12:32:19 +0100 Subject: [PATCH 02/11] Make NetworkSystem generic over the Adapter --- .../src/network/dbus/service.rs | 9 ++-- rust/agama-dbus-server/src/network/system.rs | 48 +++++++------------ 2 files changed, 21 insertions(+), 36 deletions(-) diff --git a/rust/agama-dbus-server/src/network/dbus/service.rs b/rust/agama-dbus-server/src/network/dbus/service.rs index 32513b884e..2b746dc0d0 100644 --- a/rust/agama-dbus-server/src/network/dbus/service.rs +++ b/rust/agama-dbus-server/src/network/dbus/service.rs @@ -1,7 +1,7 @@ //! Network D-Bus service. //! //! This module defines a D-Bus service which exposes Agama's network configuration. -use crate::network::NetworkSystem; +use crate::network::{nm::NetworkManagerAdapter, NetworkSystem}; use agama_lib::connection_to; use std::error::Error; @@ -15,10 +15,11 @@ impl NetworkService { pub async fn start(address: &str) -> Result<(), Box> { const SERVICE_NAME: &str = "org.opensuse.Agama.Network1"; - let connection = connection_to(address).await?; - let mut network = NetworkSystem::from_network_manager(connection.clone()) + let adapter = NetworkManagerAdapter::from_system() .await - .expect("Could not read network state"); + .expect("Could not connect to NetworkManager to read the configuration."); + let connection = connection_to(address).await?; + let mut network = NetworkSystem::new(connection.clone(), adapter); async_std::task::spawn(async move { network diff --git a/rust/agama-dbus-server/src/network/system.rs b/rust/agama-dbus-server/src/network/system.rs index d7b06c1f8e..6597f28ea1 100644 --- a/rust/agama-dbus-server/src/network/system.rs +++ b/rust/agama-dbus-server/src/network/system.rs @@ -1,53 +1,36 @@ -use crate::network::{ - dbus::Tree, model::Connection, nm::NetworkManagerAdapter, Action, Adapter, NetworkState, -}; -use agama_lib::error::ServiceError; +use crate::network::{dbus::Tree, model::Connection, Action, Adapter, NetworkState}; use async_std::channel::{unbounded, Receiver, Sender}; use std::error::Error; -/// Represents the network system, wrapping a [NetworkState] and setting up the D-Bus tree. -pub struct NetworkSystem { +/// Represents the network system using holding the state and setting up the D-Bus tree. +pub struct NetworkSystem { /// Network state pub state: NetworkState, /// Side of the channel to send actions. actions_tx: Sender, actions_rx: Receiver, tree: Tree, + /// Adapter to read/write the network state. + adapter: T, } -impl NetworkSystem { - pub fn new(state: NetworkState, conn: zbus::Connection) -> Self { +impl NetworkSystem { + pub fn new(conn: zbus::Connection, adapter: T) -> Self { let (actions_tx, actions_rx) = unbounded(); let tree = Tree::new(conn, actions_tx.clone()); Self { - state, + state: NetworkState::default(), actions_tx, actions_rx, tree, + adapter, } } - /// Reads the network configuration using the NetworkManager adapter. - /// - /// * `conn`: connection where self will be exposed. Another connection will be made internally - /// to talk with NetworkManager (which may be on a different bus even). - pub async fn from_network_manager( - conn: zbus::Connection, - ) -> Result> { - let adapter = NetworkManagerAdapter::from_system() - .await - .expect("Could not connect to NetworkManager to read the configuration."); - let state = adapter.read()?; - Ok(Self::new(state, conn)) - } - - /// Writes the network configuration to NetworkManager. - pub async fn to_network_manager(&mut self) -> Result<(), Box> { - let adapter = NetworkManagerAdapter::from_system() - .await - .expect("Could not connect to NetworkManager to write the changes."); - adapter.write(&self.state)?; - self.state = adapter.read()?; + /// Writes the network configuration. + pub async fn write(&mut self) -> Result<(), Box> { + self.adapter.write(&self.state)?; + self.state = self.adapter.read()?; Ok(()) } @@ -58,7 +41,8 @@ impl NetworkSystem { } /// Populates the D-Bus tree with the known devices and connections. - pub async fn setup(&mut self) -> Result<(), ServiceError> { + pub async fn setup(&mut self) -> Result<(), Box> { + self.state = self.adapter.read()?; self.tree .set_connections(&mut self.state.connections) .await?; @@ -93,7 +77,7 @@ impl NetworkSystem { self.state.remove_connection(&id)?; } Action::Apply => { - self.to_network_manager().await?; + self.write().await?; // TODO: re-creating the tree is kind of brute-force and it sends signals about // adding/removing interfaces. We should add/update/delete objects as needed. self.tree From ea6cf3b4e2d68f169c1d191aaf0b20e085c28218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 25 Aug 2023 12:03:10 +0100 Subject: [PATCH 03/11] Allow to inject a different adapter in the NetworkSystem --- rust/agama-dbus-server/src/main.rs | 4 ++-- rust/agama-dbus-server/src/network.rs | 7 +++++++ rust/agama-dbus-server/src/network/dbus/service.rs | 10 +++++----- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/rust/agama-dbus-server/src/main.rs b/rust/agama-dbus-server/src/main.rs index 6b3914d56c..a84b77c107 100644 --- a/rust/agama-dbus-server/src/main.rs +++ b/rust/agama-dbus-server/src/main.rs @@ -1,4 +1,4 @@ -use agama_dbus_server::{locale, network::NetworkService, questions}; +use agama_dbus_server::{locale, network, questions}; use log::LevelFilter; use std::future::pending; @@ -28,7 +28,7 @@ async fn main() -> Result<(), Box> { log::info!("Started questions interface"); let _conn = locale::start_service(ADDRESS).await?; log::info!("Started locale interface"); - NetworkService::start(ADDRESS).await?; + network::start_service(ADDRESS).await?; log::info!("Started network interface"); // Do other things or go to wait forever diff --git a/rust/agama-dbus-server/src/network.rs b/rust/agama-dbus-server/src/network.rs index 93e8dfdc8d..e7fa1d3673 100644 --- a/rust/agama-dbus-server/src/network.rs +++ b/rust/agama-dbus-server/src/network.rs @@ -51,3 +51,10 @@ pub use adapter::Adapter; pub use dbus::NetworkService; pub use model::NetworkState; pub use system::NetworkSystem; + +pub async fn start_service(address: &str) -> Result<(), Box> { + let adapter = NetworkManagerAdapter::from_system() + .await + .expect("Could not connect to NetworkManager to read the configuration."); + NetworkService::start(address, adapter).await +} diff --git a/rust/agama-dbus-server/src/network/dbus/service.rs b/rust/agama-dbus-server/src/network/dbus/service.rs index 2b746dc0d0..37057882b1 100644 --- a/rust/agama-dbus-server/src/network/dbus/service.rs +++ b/rust/agama-dbus-server/src/network/dbus/service.rs @@ -1,7 +1,7 @@ //! Network D-Bus service. //! //! This module defines a D-Bus service which exposes Agama's network configuration. -use crate::network::{nm::NetworkManagerAdapter, NetworkSystem}; +use crate::network::{Adapter, NetworkSystem}; use agama_lib::connection_to; use std::error::Error; @@ -12,12 +12,12 @@ pub struct NetworkService; impl NetworkService { /// Starts listening and dispatching events on the D-Bus connection. - pub async fn start(address: &str) -> Result<(), Box> { + pub async fn start( + address: &str, + adapter: T, + ) -> Result<(), Box> { const SERVICE_NAME: &str = "org.opensuse.Agama.Network1"; - let adapter = NetworkManagerAdapter::from_system() - .await - .expect("Could not connect to NetworkManager to read the configuration."); let connection = connection_to(address).await?; let mut network = NetworkSystem::new(connection.clone(), adapter); From 2835ca556f0f338d5cf6ce4558d94117d7f61e12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Mon, 14 Aug 2023 09:44:46 +0100 Subject: [PATCH 04/11] Write agama-dbus-server integration tests --- rust/agama-dbus-server/src/network.rs | 1 + rust/agama-dbus-server/src/network/model.rs | 2 +- rust/agama-dbus-server/tests/common/mod.rs | 49 +++++++++++ rust/agama-dbus-server/tests/network.rs | 95 +++++++++++++++++++++ rust/agama-lib/src/network.rs | 2 +- rust/share/dbus-test.conf | 82 ++++++++++++++++++ 6 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 rust/agama-dbus-server/tests/common/mod.rs create mode 100644 rust/agama-dbus-server/tests/network.rs create mode 100644 rust/share/dbus-test.conf diff --git a/rust/agama-dbus-server/src/network.rs b/rust/agama-dbus-server/src/network.rs index e7fa1d3673..1e6009ccea 100644 --- a/rust/agama-dbus-server/src/network.rs +++ b/rust/agama-dbus-server/src/network.rs @@ -50,6 +50,7 @@ pub use action::Action; pub use adapter::Adapter; pub use dbus::NetworkService; pub use model::NetworkState; +pub use nm::NetworkManagerAdapter; pub use system::NetworkSystem; pub async fn start_service(address: &str) -> Result<(), Box> { diff --git a/rust/agama-dbus-server/src/network/model.rs b/rust/agama-dbus-server/src/network/model.rs index 34937d20b3..43c6ef25c7 100644 --- a/rust/agama-dbus-server/src/network/model.rs +++ b/rust/agama-dbus-server/src/network/model.rs @@ -13,7 +13,7 @@ use std::{ }; use thiserror::Error; -#[derive(Default)] +#[derive(Default, Clone)] pub struct NetworkState { pub devices: Vec, pub connections: Vec, diff --git a/rust/agama-dbus-server/tests/common/mod.rs b/rust/agama-dbus-server/tests/common/mod.rs new file mode 100644 index 0000000000..9f1849cd68 --- /dev/null +++ b/rust/agama-dbus-server/tests/common/mod.rs @@ -0,0 +1,49 @@ +use std::process::{Child, Command}; +use uuid::Uuid; + +pub struct DBusServer { + child: Option, + pub address: String, +} + +impl DBusServer { + pub fn start_server() -> Self { + let mut server = Self::new(); + server.start(); + println!("Starting the server at {}", &server.address); + server + } + + pub fn new() -> Self { + let uuid = Uuid::new_v4(); + Self { + address: format!("unix:path=/tmp/agama-tests-{uuid}"), + child: None, + } + } + + pub fn start(&mut self) { + let child = Command::new("/usr/bin/dbus-daemon") + .args([ + "--config-file", + "../share/dbus-test.conf", + "--address", + &self.address, + ]) + .spawn() + .expect("to start the testing D-Bus daemon"); + self.child = Some(child); + } + + pub fn stop(&mut self) { + if let Some(mut child) = self.child.take() { + child.kill().unwrap(); + } + } +} + +impl Drop for DBusServer { + fn drop(&mut self) { + self.stop(); + } +} diff --git a/rust/agama-dbus-server/tests/network.rs b/rust/agama-dbus-server/tests/network.rs new file mode 100644 index 0000000000..b79e18cba3 --- /dev/null +++ b/rust/agama-dbus-server/tests/network.rs @@ -0,0 +1,95 @@ +mod common; + +use self::common::DBusServer; +use agama_dbus_server::network::{self, model, Adapter, NetworkService, NetworkState}; +use agama_lib::{ + connection_to, + network::{settings, types::DeviceType, NetworkClient}, +}; +use async_std::test; + +#[derive(Default)] +pub struct NetworkTestAdapter(network::NetworkState); + +impl Adapter for NetworkTestAdapter { + fn read(&self) -> Result> { + Ok(self.0.clone()) + } + + fn write(&self, _network: &network::NetworkState) -> Result<(), Box> { + unimplemented!("Not used in tests"); + } +} + +#[test] +async fn test_read_connections() { + let server = DBusServer::start_server(); + + let device = model::Device { + name: String::from("eth0"), + type_: DeviceType::Ethernet, + }; + let eth0 = model::Connection::new("eth0".to_string(), DeviceType::Ethernet); + let state = NetworkState::new(vec![device], vec![eth0]); + let adapter = NetworkTestAdapter(state); + + // TODO: Find a better way to detect when the server started + let ten_millis = std::time::Duration::from_millis(1000); + std::thread::sleep(ten_millis); + + let _service = NetworkService::start(&server.address, adapter) + .await + .unwrap(); + + let ten_millis = std::time::Duration::from_millis(1000); + std::thread::sleep(ten_millis); + + let connection = connection_to(&server.address).await.unwrap(); + let client = NetworkClient::new(connection.clone()).await.unwrap(); + let conns = client.connections().await.unwrap(); + assert_eq!(conns.len(), 1); + let dbus_eth0 = conns.first().unwrap(); + assert_eq!(dbus_eth0.id, "eth0"); + assert_eq!(dbus_eth0.device_type(), DeviceType::Ethernet); +} + +#[test] +async fn test_add_connection() { + let server = DBusServer::start_server(); + + let state = NetworkState::default(); + let adapter = NetworkTestAdapter(state); + + // TODO: Find a better way to detect when the server started + let ten_millis = std::time::Duration::from_millis(100); + std::thread::sleep(ten_millis); + + let _service = NetworkService::start(&server.address, adapter) + .await + .unwrap(); + + let ten_millis = std::time::Duration::from_millis(1000); + std::thread::sleep(ten_millis); + + let connection = connection_to(&server.address).await.unwrap(); + let client = NetworkClient::new(connection.clone()).await.unwrap(); + + let wlan0 = settings::NetworkConnection { + id: "wlan0".to_string(), + method: Some("auto".to_string()), + wireless: Some(settings::WirelessSettings { + password: "123456".to_string(), + security: "wpa-psk".to_string(), + ssid: "TEST".to_string(), + mode: "infrastructure".to_string(), + }), + ..Default::default() + }; + client.add_or_update_connection(&wlan0).await.unwrap(); + + let conns = client.connections().await.unwrap(); + assert_eq!(conns.len(), 1); + let conn = conns.first().unwrap(); + assert_eq!(conn.id, "wlan0"); + assert_eq!(conn.device_type(), DeviceType::Wireless); +} diff --git a/rust/agama-lib/src/network.rs b/rust/agama-lib/src/network.rs index 11bd289408..0f0d65674a 100644 --- a/rust/agama-lib/src/network.rs +++ b/rust/agama-lib/src/network.rs @@ -2,7 +2,7 @@ mod client; mod proxies; -mod settings; +pub mod settings; mod store; pub mod types; diff --git a/rust/share/dbus-test.conf b/rust/share/dbus-test.conf new file mode 100644 index 0000000000..7cc5329c72 --- /dev/null +++ b/rust/share/dbus-test.conf @@ -0,0 +1,82 @@ + + + + org.opensuse.Agama + + + + + unix:tmpdir=/tmp + + + EXTERNAL + + + + + + + + + + + + + + + + + + + + + + + + + + + + contexts/dbus_contexts + + + + + 1000000000 + 250000000 + 1000000000 + 250000000 + 1000000000 + + 120000 + 240000 + 150000 + 100000 + 10000 + 100000 + 10000 + 50000 + 50000 + 50000 + From 8f8c5d07bc6b15d82f24b26c4118d51fcc87740e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 25 Aug 2023 12:18:39 +0100 Subject: [PATCH 05/11] Document a potential bug in the network service --- rust/agama-lib/src/network/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/agama-lib/src/network/client.rs b/rust/agama-lib/src/network/client.rs index a849c1b164..8e0fe74351 100644 --- a/rust/agama-lib/src/network/client.rs +++ b/rust/agama-lib/src/network/client.rs @@ -123,6 +123,7 @@ impl<'a> NetworkClient<'a> { self.connections_proxy .add_connection(&conn.id, conn.device_type() as u8) .await?; + // FIXME: this method might not work because the object might time some time to appear. Ok(self.connections_proxy.get_connection(&conn.id).await?) } From 00840d15354c8cb9c99a554a4daa6d9e93263bba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 25 Aug 2023 12:49:26 +0100 Subject: [PATCH 06/11] Fix NetworkClient to wait for the connection to appear --- rust/agama-lib/src/network/client.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/rust/agama-lib/src/network/client.rs b/rust/agama-lib/src/network/client.rs index 8e0fe74351..cd1d5b11e5 100644 --- a/rust/agama-lib/src/network/client.rs +++ b/rust/agama-lib/src/network/client.rs @@ -123,8 +123,7 @@ impl<'a> NetworkClient<'a> { self.connections_proxy .add_connection(&conn.id, conn.device_type() as u8) .await?; - // FIXME: this method might not work because the object might time some time to appear. - Ok(self.connections_proxy.get_connection(&conn.id).await?) + self.wait_for_connection(&conn.id).await } /// Updates a network connection. @@ -182,4 +181,19 @@ impl<'a> NetworkClient<'a> { proxy.set_password(&wireless.password).await?; Ok(()) } + + async fn wait_for_connection(&self, id: &str) -> Result { + loop { + let mut retries = 0; + match self.connections_proxy.get_connection(&id).await { + Ok(path) => return Ok(path), + Err(e) => { + retries += 1; + if retries > 3 { + return Err(e.into()); + }; + } + } + } + } } From 85ec299dc05fd95887b71492c5afbb88788a8d04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 25 Aug 2023 14:17:22 +0100 Subject: [PATCH 07/11] Use a signal to detect when a connection is added --- .../src/network/dbus/interfaces.rs | 10 ++++- .../src/network/dbus/tree.rs | 23 +++++++++- rust/agama-dbus-server/src/network/system.rs | 2 +- rust/agama-lib/src/network/client.rs | 43 ++++++++++++------- rust/agama-lib/src/network/proxies.rs | 4 ++ 5 files changed, 63 insertions(+), 19 deletions(-) diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces.rs b/rust/agama-dbus-server/src/network/dbus/interfaces.rs index 6af95d197f..b8055fa156 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces.rs @@ -19,6 +19,7 @@ use std::net::{AddrParseError, Ipv4Addr}; use zbus::{ dbus_interface, zvariant::{ObjectPath, OwnedObjectPath}, + SignalContext, }; /// D-Bus interface for the network devices collection @@ -126,7 +127,7 @@ impl Connections { pub async fn add_connection(&mut self, id: String, ty: u8) -> zbus::fdo::Result<()> { let actions = self.actions.lock().await; actions - .send(Action::AddConnection(id, ty.try_into()?)) + .send(Action::AddConnection(id.clone(), ty.try_into()?)) .await .unwrap(); Ok(()) @@ -163,6 +164,13 @@ impl Connections { actions.send(Action::Apply).await.unwrap(); Ok(()) } + + #[dbus_interface(signal)] + pub async fn connection_added( + ctxt: &SignalContext<'_>, + id: &str, + path: &str, + ) -> zbus::Result<()>; } /// D-Bus interface for a network connection diff --git a/rust/agama-dbus-server/src/network/dbus/tree.rs b/rust/agama-dbus-server/src/network/dbus/tree.rs index 438086b607..9e09c02aa2 100644 --- a/rust/agama-dbus-server/src/network/dbus/tree.rs +++ b/rust/agama-dbus-server/src/network/dbus/tree.rs @@ -79,9 +79,15 @@ impl Tree { /// Adds a connection to the D-Bus tree. /// /// * `connection`: connection to add. - pub async fn add_connection(&self, conn: &mut Connection) -> Result<(), ServiceError> { + /// * `notify`: whether to notify the added connection + pub async fn add_connection( + &self, + conn: &mut Connection, + notify: bool, + ) -> Result<(), ServiceError> { let mut objects = self.objects.lock().await; + let orig_id = conn.id().to_owned(); let (id, path) = objects.register_connection(conn); if id != conn.id() { conn.set_id(&id) @@ -106,6 +112,10 @@ impl Tree { .await?; } + if notify { + self.notify_connection_added(&orig_id, &path).await?; + } + Ok(()) } @@ -127,7 +137,7 @@ impl Tree { /// * `connections`: list of connections. async fn add_connections(&self, connections: &mut [Connection]) -> Result<(), ServiceError> { for conn in connections.iter_mut() { - self.add_connection(conn).await?; + self.add_connection(conn, false).await?; } self.add_interface( @@ -182,6 +192,15 @@ impl Tree { let object_server = self.connection.object_server(); Ok(object_server.at(path, iface).await?) } + + /// Notify that a new connection has been added + async fn notify_connection_added(&self, id: &str, path: &str) -> Result<(), ServiceError> { + let object_server = self.connection.object_server(); + let iface_ref = object_server + .interface::<_, interfaces::Connections>(CONNECTIONS_PATH) + .await?; + Ok(interfaces::Connections::connection_added(iface_ref.signal_context(), id, path).await?) + } } /// Objects paths for known devices and connections diff --git a/rust/agama-dbus-server/src/network/system.rs b/rust/agama-dbus-server/src/network/system.rs index 6597f28ea1..5a1fa40879 100644 --- a/rust/agama-dbus-server/src/network/system.rs +++ b/rust/agama-dbus-server/src/network/system.rs @@ -66,7 +66,7 @@ impl NetworkSystem { match action { Action::AddConnection(name, ty) => { let mut conn = Connection::new(name, ty); - self.tree.add_connection(&mut conn).await?; + self.tree.add_connection(&mut conn, true).await?; self.state.add_connection(conn)?; } Action::UpdateConnection(conn) => { diff --git a/rust/agama-lib/src/network/client.rs b/rust/agama-lib/src/network/client.rs index cd1d5b11e5..a281355de7 100644 --- a/rust/agama-lib/src/network/client.rs +++ b/rust/agama-lib/src/network/client.rs @@ -3,6 +3,7 @@ use super::types::SSID; use crate::error::ServiceError; use super::proxies::{ConnectionProxy, ConnectionsProxy, IPv4Proxy, WirelessProxy}; +use async_std::stream::StreamExt; use zbus::zvariant::OwnedObjectPath; use zbus::Connection; @@ -120,10 +121,21 @@ impl<'a> NetworkClient<'a> { &self, conn: &NetworkConnection, ) -> Result { + let mut stream = self.connections_proxy.receive_connection_added().await?; + self.connections_proxy .add_connection(&conn.id, conn.device_type() as u8) .await?; - self.wait_for_connection(&conn.id).await + + loop { + let signal = stream.next().await.unwrap(); + let (id, _path): (String, String) = signal.body().unwrap(); + if id == conn.id { + break; + }; + } + + Ok(self.connections_proxy.get_connection(&conn.id).await?) } /// Updates a network connection. @@ -182,18 +194,19 @@ impl<'a> NetworkClient<'a> { Ok(()) } - async fn wait_for_connection(&self, id: &str) -> Result { - loop { - let mut retries = 0; - match self.connections_proxy.get_connection(&id).await { - Ok(path) => return Ok(path), - Err(e) => { - retries += 1; - if retries > 3 { - return Err(e.into()); - }; - } - } - } - } + // Replace with a better implemenation based on signals. + // async fn wait_for_connection(&self, id: &str) -> Result { + // loop { + // let mut retries = 0; + // match self.connections_proxy.get_connection(&id).await { + // Ok(path) => return Ok(path), + // Err(e) => { + // retries += 1; + // if retries > 3 { + // return Err(e.into()); + // }; + // } + // } + // } + // } } diff --git a/rust/agama-lib/src/network/proxies.rs b/rust/agama-lib/src/network/proxies.rs index 5b787ca677..4166abeaa8 100644 --- a/rust/agama-lib/src/network/proxies.rs +++ b/rust/agama-lib/src/network/proxies.rs @@ -28,6 +28,10 @@ trait Connections { /// RemoveConnection method fn remove_connection(&self, uuid: &str) -> zbus::Result<()>; + + /// ConnectionAdded signal + #[dbus_proxy(signal)] + fn connection_added(&self, id: &str, path: &str) -> zbus::Result<()>; } #[dbus_proxy( From f22a960b371f7d4e6928ce2b53e20b54141eef0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Mon, 28 Aug 2023 13:23:30 +0100 Subject: [PATCH 08/11] Tests: detect when a service is started --- rust/agama-dbus-server/tests/common/mod.rs | 62 ++++++++++++++++++++-- rust/agama-dbus-server/tests/network.rs | 20 ++----- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/rust/agama-dbus-server/tests/common/mod.rs b/rust/agama-dbus-server/tests/common/mod.rs index 9f1849cd68..883f4ade47 100644 --- a/rust/agama-dbus-server/tests/common/mod.rs +++ b/rust/agama-dbus-server/tests/common/mod.rs @@ -1,28 +1,42 @@ +use agama_lib::{connection_to, error::ServiceError}; +use async_std::stream::StreamExt; use std::process::{Child, Command}; use uuid::Uuid; +use zbus::{MatchRule, MessageStream, MessageType}; +/// D-Bus server to be used on tests. +/// +/// This struct takes care of starting, stopping and monitoring dbus-daemon to be used on +/// integration tests. Each server uses a different socket, so they do not collide. pub struct DBusServer { child: Option, + messages: Option, pub address: String, } impl DBusServer { - pub fn start_server() -> Self { + /// Builds and starts a server. + pub async fn start_server() -> Result { let mut server = Self::new(); - server.start(); + server.start().await?; println!("Starting the server at {}", &server.address); - server + Ok(server) } + /// Builds a new server. + /// + /// To start the server, check the `start` function. pub fn new() -> Self { let uuid = Uuid::new_v4(); Self { address: format!("unix:path=/tmp/agama-tests-{uuid}"), child: None, + messages: None, } } - pub fn start(&mut self) { + /// Starts the server. + pub async fn start(&mut self) -> Result<(), ServiceError> { let child = Command::new("/usr/bin/dbus-daemon") .args([ "--config-file", @@ -33,12 +47,52 @@ impl DBusServer { .spawn() .expect("to start the testing D-Bus daemon"); self.child = Some(child); + self.wait(); + self.messages = Some(self.build_message_stream().await?); + Ok(()) } + /// Stops the server. pub fn stop(&mut self) { if let Some(mut child) = self.child.take() { child.kill().unwrap(); } + self.messages = None; + } + + /// Waits for a server to be available. + /// + /// * `name`: service name (e.g., "org.opensuse.Agama.Network1"). + pub async fn wait_for_service(&mut self, name: &str) { + let Some(ref mut messages) = self.messages else { + return; + }; + + loop { + let signal = messages.next().await.unwrap().unwrap(); + let (sname, _, _): (String, String, String) = signal.body().unwrap(); + if &sname == name { + return; + } + } + } + + /// Waits until the D-Bus daemon is ready. + // TODO: implement proper waiting instead of just using a sleep + fn wait(&self) { + let wait_time = std::time::Duration::from_millis(500); + std::thread::sleep(wait_time); + } + + /// Builds a message stream. + async fn build_message_stream(&self) -> Result { + let conn = connection_to(&self.address).await?; + let rule = MatchRule::builder() + .msg_type(MessageType::Signal) + .sender("org.freedesktop.DBus")? + .member("NameOwnerChanged")? + .build(); + Ok(MessageStream::for_match_rule(rule, &conn, None).await?) } } diff --git a/rust/agama-dbus-server/tests/network.rs b/rust/agama-dbus-server/tests/network.rs index b79e18cba3..43d6877aa6 100644 --- a/rust/agama-dbus-server/tests/network.rs +++ b/rust/agama-dbus-server/tests/network.rs @@ -23,7 +23,7 @@ impl Adapter for NetworkTestAdapter { #[test] async fn test_read_connections() { - let server = DBusServer::start_server(); + let mut server = DBusServer::start_server().await.unwrap(); let device = model::Device { name: String::from("eth0"), @@ -33,16 +33,10 @@ async fn test_read_connections() { let state = NetworkState::new(vec![device], vec![eth0]); let adapter = NetworkTestAdapter(state); - // TODO: Find a better way to detect when the server started - let ten_millis = std::time::Duration::from_millis(1000); - std::thread::sleep(ten_millis); - let _service = NetworkService::start(&server.address, adapter) .await .unwrap(); - - let ten_millis = std::time::Duration::from_millis(1000); - std::thread::sleep(ten_millis); + server.wait_for_service("org.opensuse.Agama.Network1").await; let connection = connection_to(&server.address).await.unwrap(); let client = NetworkClient::new(connection.clone()).await.unwrap(); @@ -55,21 +49,15 @@ async fn test_read_connections() { #[test] async fn test_add_connection() { - let server = DBusServer::start_server(); + let mut server = DBusServer::start_server().await.unwrap(); let state = NetworkState::default(); let adapter = NetworkTestAdapter(state); - // TODO: Find a better way to detect when the server started - let ten_millis = std::time::Duration::from_millis(100); - std::thread::sleep(ten_millis); - let _service = NetworkService::start(&server.address, adapter) .await .unwrap(); - - let ten_millis = std::time::Duration::from_millis(1000); - std::thread::sleep(ten_millis); + server.wait_for_service("org.opensuse.Agama.Network1").await; let connection = connection_to(&server.address).await.unwrap(); let client = NetworkClient::new(connection.clone()).await.unwrap(); From c867656b152f6f8497857e51da5a00e48e1c30b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Mon, 28 Aug 2023 13:50:38 +0100 Subject: [PATCH 09/11] Run "cargo fmt" happy according to the CI --- rust/agama-dbus-server/src/network/dbus/tree.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/agama-dbus-server/src/network/dbus/tree.rs b/rust/agama-dbus-server/src/network/dbus/tree.rs index 9e09c02aa2..6878c78a96 100644 --- a/rust/agama-dbus-server/src/network/dbus/tree.rs +++ b/rust/agama-dbus-server/src/network/dbus/tree.rs @@ -125,7 +125,7 @@ impl Tree { pub async fn remove_connection(&mut self, id: &str) -> Result<(), ServiceError> { let mut objects = self.objects.lock().await; let Some(path) = objects.connection_path(id) else { - return Ok(()) + return Ok(()); }; self.remove_connection_on(path.as_str()).await?; objects.deregister_connection(id).unwrap(); From 4dd1ad035c7585beea1e8dc612aa481128a8d4a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Mon, 28 Aug 2023 14:10:42 +0100 Subject: [PATCH 10/11] Remove commented (and outdated) code --- rust/agama-lib/src/network/client.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/rust/agama-lib/src/network/client.rs b/rust/agama-lib/src/network/client.rs index a281355de7..240d0359f0 100644 --- a/rust/agama-lib/src/network/client.rs +++ b/rust/agama-lib/src/network/client.rs @@ -193,20 +193,4 @@ impl<'a> NetworkClient<'a> { proxy.set_password(&wireless.password).await?; Ok(()) } - - // Replace with a better implemenation based on signals. - // async fn wait_for_connection(&self, id: &str) -> Result { - // loop { - // let mut retries = 0; - // match self.connections_proxy.get_connection(&id).await { - // Ok(path) => return Ok(path), - // Err(e) => { - // retries += 1; - // if retries > 3 { - // return Err(e.into()); - // }; - // } - // } - // } - // } } From 601135f8f3da5da27bcdd369a7c745b6c32b6f5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Mon, 28 Aug 2023 14:26:06 +0100 Subject: [PATCH 11/11] Update from code review --- rust/agama-dbus-server/tests/common/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rust/agama-dbus-server/tests/common/mod.rs b/rust/agama-dbus-server/tests/common/mod.rs index 883f4ade47..2fb8031397 100644 --- a/rust/agama-dbus-server/tests/common/mod.rs +++ b/rust/agama-dbus-server/tests/common/mod.rs @@ -1,6 +1,7 @@ use agama_lib::{connection_to, error::ServiceError}; use async_std::stream::StreamExt; use std::process::{Child, Command}; +use std::time::Duration; use uuid::Uuid; use zbus::{MatchRule, MessageStream, MessageType}; @@ -80,8 +81,8 @@ impl DBusServer { /// Waits until the D-Bus daemon is ready. // TODO: implement proper waiting instead of just using a sleep fn wait(&self) { - let wait_time = std::time::Duration::from_millis(500); - std::thread::sleep(wait_time); + const WAIT_TIME: Duration = Duration::from_millis(500); + std::thread::sleep(WAIT_TIME); } /// Builds a message stream.