From 79e1a075261b1167993b9b356e18f63ee862ca1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Sun, 10 Dec 2023 19:40:04 +0000 Subject: [PATCH 1/2] Refactor Connection to use a simplified layout * Connection is now a struct containing the common fields. * The connection holds specific configuration in the "config" field. * The ConnectionConfig is an enum which models specific configurations. * Adapt the rest of the code to the new API, including the tests. --- .../src/network/dbus/interfaces.rs | 146 ++++---- .../src/network/dbus/tree.rs | 4 +- rust/agama-dbus-server/src/network/model.rs | 354 +++++++----------- rust/agama-dbus-server/src/network/nm/dbus.rs | 99 ++--- 4 files changed, 257 insertions(+), 346 deletions(-) diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces.rs b/rust/agama-dbus-server/src/network/dbus/interfaces.rs index 0467efc8a6..cdce1cc67a 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces.rs @@ -8,8 +8,8 @@ use crate::network::{ action::Action, error::NetworkStateError, model::{ - BondConnection, Connection as NetworkConnection, Device as NetworkDevice, MacAddress, - WirelessConnection, + BondConfig, Connection as NetworkConnection, ConnectionConfig, Device as NetworkDevice, + MacAddress, SecurityProtocol, WirelessConfig, WirelessMode, }, }; @@ -339,28 +339,39 @@ impl Bond { } } - /// Gets the bond connection. - /// - /// Beware that it crashes when it is not a bond connection. - async fn get_bond(&self) -> BondConnection { + /// Gets the connection. + async fn get_connection(&self) -> Result { let actions = self.actions.lock().await; let (tx, rx) = oneshot::channel(); actions.send(Action::GetConnection(self.uuid, tx)).unwrap(); - let connection = rx.await.unwrap(); + rx.await.unwrap().ok_or(NetworkStateError::UnknownConnection(self.uuid.to_string())) + } - match connection { - Some(NetworkConnection::Bond(config)) => config, + /// Gets the bonding configuration. + pub async fn get_config(&self) -> Result { + let connection = self.get_connection().await?; + match connection.config { + ConnectionConfig::Bond(bond) => Ok(bond), _ => panic!("Not a bond connection. This is most probably a bug."), } } - /// Updates the connection data in the NetworkSystem. + /// Updates the bond configuration. /// - /// * `connection`: Updated connection. - async fn update_connection<'a>(&self, connection: BondConnection) -> zbus::fdo::Result<()> { + /// * `func`: function to update the configuration. + pub async fn update_config(&self, func: F) -> Result<(), NetworkStateError> + where + F: FnOnce(&mut BondConfig), + { + let mut connection = self.get_connection().await?; + match &mut connection.config { + ConnectionConfig::Bond(bond) => func(bond), + _ => panic!("Not a bond connection. This is most probably a bug."), + } let actions = self.actions.lock().await; - let connection = NetworkConnection::Bond(connection.clone()); - actions.send(Action::UpdateConnection(connection)).unwrap(); + actions + .send(Action::UpdateConnection(connection.clone())) + .unwrap(); Ok(()) } } @@ -369,30 +380,30 @@ impl Bond { impl Bond { /// Bonding mode. #[dbus_interface(property)] - pub async fn mode(&self) -> String { - let connection = self.get_bond().await; - connection.bond.mode.to_string() + pub async fn mode(&self) -> zbus::fdo::Result { + let config = self.get_config().await?; + Ok(config.mode.to_string()) } #[dbus_interface(property)] pub async fn set_mode(&mut self, mode: &str) -> zbus::fdo::Result<()> { - let mut connection = self.get_bond().await; - connection.set_mode(mode.try_into()?); - self.update_connection(connection).await + let mode = mode.try_into()?; + self.update_config(|c| c.mode = mode).await?; + Ok(()) } /// List of bonding options. #[dbus_interface(property)] - pub async fn options(&self) -> String { - let connection = self.get_bond().await; - connection.bond.options.to_string() + pub async fn options(&self) -> zbus::fdo::Result { + let config = self.get_config().await?; + Ok(config.options.to_string()) } #[dbus_interface(property)] pub async fn set_options(&mut self, opts: &str) -> zbus::fdo::Result<()> { - let mut connection = self.get_bond().await; - connection.set_options(opts.try_into()?); - self.update_connection(connection).await + let opts = opts.try_into()?; + self.update_config(|c| c.options = opts).await?; + Ok(()) } /// List of bond ports. @@ -507,23 +518,29 @@ impl Wireless { /// Gets the wireless connection. /// /// Beware that it crashes when it is not a wireless connection. - async fn get_wireless(&self) -> MappedMutexGuard { - MutexGuard::map(self.connection.lock().await, |c| match c { - NetworkConnection::Wireless(config) => config, - _ => panic!("Not a wireless network. This is most probably a bug."), + async fn get_wireless(&self) -> MappedMutexGuard { + MutexGuard::map(self.connection.lock().await, |c| match &mut c.config { + ConnectionConfig::Wireless(config) => config, + _ => panic!("Not a wireless connection. This is most probably a bug."), }) } - /// Updates the connection data in the NetworkSystem. + /// Updates the wireless configuration. /// - /// * `connection`: Updated connection. - async fn update_connection<'a>( - &self, - connection: MappedMutexGuard<'a, WirelessConnection>, - ) -> zbus::fdo::Result<()> { + /// * `func`: function to update the configuration. + pub async fn update_config(&self, func: F) -> Result<(), NetworkStateError> + where + F: FnOnce(&mut WirelessConfig), + { + let mut connection = self.connection.lock().await; + match &mut connection.config { + ConnectionConfig::Wireless(wireless) => func(wireless), + _ => panic!("Not a wireless connection. This is most probably a bug."), + } let actions = self.actions.lock().await; - let connection = NetworkConnection::Wireless(connection.clone()); - actions.send(Action::UpdateConnection(connection)).unwrap(); + actions + .send(Action::UpdateConnection(connection.clone())) + .unwrap(); Ok(()) } } @@ -533,15 +550,14 @@ impl Wireless { /// Network SSID. #[dbus_interface(property, name = "SSID")] pub async fn ssid(&self) -> Vec { - let connection = self.get_wireless().await; - connection.wireless.ssid.clone().into() + let wireless = self.get_wireless().await; + wireless.ssid.clone().into() } #[dbus_interface(property, name = "SSID")] pub async fn set_ssid(&mut self, ssid: Vec) -> zbus::fdo::Result<()> { - let mut connection = self.get_wireless().await; - connection.wireless.ssid = SSID(ssid); - self.update_connection(connection).await + self.update_config(|c| c.ssid = SSID(ssid)).await?; + Ok(()) } /// Wireless connection mode. @@ -551,37 +567,35 @@ impl Wireless { /// See [crate::network::model::WirelessMode]. #[dbus_interface(property)] pub async fn mode(&self) -> String { - let connection = self.get_wireless().await; - connection.wireless.mode.to_string() + let wireless = self.get_wireless().await; + wireless.mode.to_string() } #[dbus_interface(property)] pub async fn set_mode(&mut self, mode: &str) -> zbus::fdo::Result<()> { - let mut connection = self.get_wireless().await; - connection.wireless.mode = mode.try_into()?; - self.update_connection(connection).await + let mode: WirelessMode = mode.try_into()?; + self.update_config(|c| c.mode = mode).await?; + Ok(()) } /// Password to connect to the wireless network. #[dbus_interface(property)] pub async fn password(&self) -> String { - let connection = self.get_wireless().await; - connection - .wireless - .password - .clone() - .unwrap_or("".to_string()) + let wireless = self.get_wireless().await; + wireless.password.clone().unwrap_or("".to_string()) } #[dbus_interface(property)] pub async fn set_password(&mut self, password: String) -> zbus::fdo::Result<()> { - let mut connection = self.get_wireless().await; - connection.wireless.password = if password.is_empty() { - None - } else { - Some(password) - }; - self.update_connection(connection).await + self.update_config(|c| { + c.password = if password.is_empty() { + None + } else { + Some(password) + }; + }) + .await?; + Ok(()) } /// Wireless security protocol. @@ -592,16 +606,16 @@ impl Wireless { /// See [crate::network::model::SecurityProtocol]. #[dbus_interface(property)] pub async fn security(&self) -> String { - let connection = self.get_wireless().await; - connection.wireless.security.to_string() + let wireless = self.get_wireless().await; + wireless.security.to_string() } #[dbus_interface(property)] pub async fn set_security(&mut self, security: &str) -> zbus::fdo::Result<()> { - let mut connection = self.get_wireless().await; - connection.wireless.security = security + let security: SecurityProtocol = security .try_into() .map_err(|_| NetworkStateError::InvalidSecurityProtocol(security.to_string()))?; - self.update_connection(connection).await + self.update_config(|c| c.security = security).await?; + Ok(()) } } diff --git a/rust/agama-dbus-server/src/network/dbus/tree.rs b/rust/agama-dbus-server/src/network/dbus/tree.rs index f2e9f0263e..9d2f3d3b97 100644 --- a/rust/agama-dbus-server/src/network/dbus/tree.rs +++ b/rust/agama-dbus-server/src/network/dbus/tree.rs @@ -114,12 +114,12 @@ impl Tree { ) .await?; - if let Connection::Bond(_) = conn { + if let ConnectionConfig::Bond(_) = conn.config { self.add_interface(&path, interfaces::Bond::new(self.actions.clone(), uuid)) .await?; } - if let Connection::Wireless(_) = conn { + if let ConnectionConfig::Wireless(_) = conn.config { self.add_interface( &path, interfaces::Wireless::new(self.actions.clone(), Arc::clone(&cloned)), diff --git a/rust/agama-dbus-server/src/network/model.rs b/rust/agama-dbus-server/src/network/model.rs index e1d53d2f5b..e4c0ce64e7 100644 --- a/rust/agama-dbus-server/src/network/model.rs +++ b/rust/agama-dbus-server/src/network/model.rs @@ -15,8 +15,6 @@ use std::{ use thiserror::Error; use uuid::Uuid; use zbus::zvariant::Value; -mod builder; -pub use builder::ConnectionBuilder; #[derive(Default, Clone, Debug)] pub struct NetworkState { @@ -129,7 +127,7 @@ impl NetworkState { controller: &Connection, ports: Vec, ) -> Result<(), NetworkStateError> { - if let Connection::Bond(_) = &controller { + if let ConnectionConfig::Bond(_) = &controller.config { let mut controlled = vec![]; for port in ports { let connection = self @@ -157,7 +155,6 @@ impl NetworkState { #[cfg(test)] mod tests { - use super::builder::ConnectionBuilder; use super::*; use crate::network::error::NetworkStateError; use uuid::Uuid; @@ -166,12 +163,11 @@ mod tests { fn test_add_connection() { let mut state = NetworkState::default(); let uuid = Uuid::new_v4(); - let base = BaseConnection { + let conn0 = Connection { id: "eth0".to_string(), uuid, ..Default::default() }; - let conn0 = Connection::Ethernet(EthernetConnection { base }); state.add_connection(conn0).unwrap(); let found = state.get_connection("eth0").unwrap(); assert_eq!(found.uuid(), uuid); @@ -180,12 +176,8 @@ mod tests { #[test] fn test_add_duplicated_connection() { let mut state = NetworkState::default(); - let uuid = Uuid::new_v4(); - let base = BaseConnection { - uuid, - ..Default::default() - }; - let conn0 = Connection::Ethernet(EthernetConnection { base }); + let mut conn0 = Connection::new("eth0".to_string(), DeviceType::Ethernet); + conn0.uuid = Uuid::new_v4(); state.add_connection(conn0.clone()).unwrap(); let error = state.add_connection(conn0).unwrap_err(); assert!(matches!(error, NetworkStateError::ConnectionExists(_))); @@ -194,22 +186,20 @@ mod tests { #[test] fn test_update_connection() { let mut state = NetworkState::default(); - let base0 = BaseConnection { + let conn0 = Connection { id: "eth0".to_string(), uuid: Uuid::new_v4(), ..Default::default() }; - let conn0 = Connection::Ethernet(EthernetConnection { base: base0 }); state.add_connection(conn0).unwrap(); let uuid = Uuid::new_v4(); - let base1 = BaseConnection { + let conn1 = Connection { id: "eth0".to_string(), uuid, ..Default::default() }; - let conn2 = Connection::Ethernet(EthernetConnection { base: base1 }); - state.update_connection(conn2).unwrap(); + state.update_connection(conn1).unwrap(); let found = state.get_connection("eth0").unwrap(); assert_eq!(found.uuid(), uuid); } @@ -217,12 +207,7 @@ mod tests { #[test] fn test_update_unknown_connection() { let mut state = NetworkState::default(); - let uuid = Uuid::new_v4(); - let base = BaseConnection { - uuid, - ..Default::default() - }; - let conn0 = Connection::Ethernet(EthernetConnection { base }); + let conn0 = Connection::new("eth0".to_string(), DeviceType::Ethernet); let error = state.update_connection(conn0).unwrap_err(); assert!(matches!(error, NetworkStateError::UnknownConnection(_))); } @@ -231,13 +216,7 @@ mod tests { fn test_remove_connection() { let mut state = NetworkState::default(); let id = "eth0".to_string(); - let uuid = Uuid::new_v4(); - let base0 = BaseConnection { - id, - uuid, - ..Default::default() - }; - let conn0 = Connection::Ethernet(EthernetConnection { base: base0 }); + let conn0 = Connection::new(id, DeviceType::Ethernet); state.add_connection(conn0).unwrap(); state.remove_connection("eth0").unwrap(); let found = state.get_connection("eth0").unwrap(); @@ -253,33 +232,32 @@ mod tests { #[test] fn test_is_loopback() { - let base = BaseConnection { - id: "eth0".to_string(), - ..Default::default() - }; - let conn = Connection::Ethernet(EthernetConnection { base }); + let conn = Connection::new("eth0".to_string(), DeviceType::Ethernet); assert!(!conn.is_loopback()); - let base = BaseConnection { - id: "lo".to_string(), - ..Default::default() - }; - let conn = Connection::Loopback(LoopbackConnection { base }); + let conn = Connection::new("eth0".to_string(), DeviceType::Loopback); assert!(conn.is_loopback()); } #[test] fn test_set_bonding_ports() { let mut state = NetworkState::default(); - let eth0 = ConnectionBuilder::new("eth0") - .with_interface("eth0") - .build(); - let eth1 = ConnectionBuilder::new("eth1") - .with_interface("eth1") - .build(); - let bond0 = ConnectionBuilder::new("bond0") - .with_type(DeviceType::Bond) - .build(); + let eth0 = Connection { + id: "eth0".to_string(), + interface: Some("eth0".to_string()), + ..Default::default() + }; + let eth1 = Connection { + id: "eth1".to_string(), + interface: Some("eth1".to_string()), + ..Default::default() + }; + let bond0 = Connection { + id: "bond0".to_string(), + interface: Some("bond0".to_string()), + config: ConnectionConfig::Bond(Default::default()), + ..Default::default() + }; state.add_connection(eth0).unwrap(); state.add_connection(eth1).unwrap(); @@ -296,9 +274,12 @@ mod tests { #[test] fn test_set_bonding_missing_port() { let mut state = NetworkState::default(); - let bond0 = ConnectionBuilder::new("bond0") - .with_type(DeviceType::Bond) - .build(); + let bond0 = Connection { + id: "bond0".to_string(), + interface: Some("bond0".to_string()), + config: ConnectionConfig::Bond(Default::default()), + ..Default::default() + }; state.add_connection(bond0.clone()).unwrap(); let error = state @@ -311,7 +292,10 @@ mod tests { #[test] fn test_set_non_controller_ports() { let mut state = NetworkState::default(); - let eth0 = ConnectionBuilder::new("eth0").build(); + let eth0 = Connection { + id: "eth0".to_string(), + ..Default::default() + }; state.add_connection(eth0.clone()).unwrap(); let error = state @@ -332,168 +316,143 @@ pub struct Device { } /// Represents an available network connection -#[derive(Debug, PartialEq, Clone)] -pub enum Connection { - Ethernet(EthernetConnection), - Wireless(WirelessConnection), - Loopback(LoopbackConnection), - Dummy(DummyConnection), - Bond(BondConnection), +#[derive(Debug, Default, Clone)] +pub struct Connection { + pub id: String, + pub uuid: Uuid, + pub mac_address: MacAddress, + pub ip_config: IpConfig, + pub status: Status, + pub interface: Option, + pub controller: Option, + pub match_config: MatchConfig, + pub config: ConnectionConfig, +} + +impl PartialEq for Connection { + fn eq(&self, other: &Self) -> bool { + self.id == other.id && self.uuid == other.uuid && self.ip_config == other.ip_config + } } impl Connection { pub fn new(id: String, device_type: DeviceType) -> Self { - let base = BaseConnection { + let config = match device_type { + DeviceType::Ethernet => ConnectionConfig::Ethernet, + DeviceType::Wireless => ConnectionConfig::Wireless(Default::default()), + DeviceType::Loopback => ConnectionConfig::Loopback, + DeviceType::Dummy => ConnectionConfig::Dummy, + DeviceType::Bond => ConnectionConfig::Bond(Default::default()), + }; + Self { id, - uuid: Uuid::new_v4(), + config, ..Default::default() - }; - match device_type { - DeviceType::Wireless => Connection::Wireless(WirelessConnection { - base, - ..Default::default() - }), - DeviceType::Loopback => Connection::Loopback(LoopbackConnection { base }), - DeviceType::Ethernet => Connection::Ethernet(EthernetConnection { base }), - DeviceType::Dummy => Connection::Dummy(DummyConnection { base }), - DeviceType::Bond => Connection::Bond(BondConnection { - base, - ..Default::default() - }), - } - } - - /// TODO: implement a macro to reduce the amount of repetitive code. The same applies to - /// the base_mut function. - pub fn base(&self) -> &BaseConnection { - match &self { - Connection::Ethernet(conn) => &conn.base, - Connection::Wireless(conn) => &conn.base, - Connection::Loopback(conn) => &conn.base, - Connection::Dummy(conn) => &conn.base, - Connection::Bond(conn) => &conn.base, - } - } - - pub fn base_mut(&mut self) -> &mut BaseConnection { - match self { - Connection::Ethernet(conn) => &mut conn.base, - Connection::Wireless(conn) => &mut conn.base, - Connection::Loopback(conn) => &mut conn.base, - Connection::Dummy(conn) => &mut conn.base, - Connection::Bond(conn) => &mut conn.base, } } pub fn id(&self) -> &str { - self.base().id.as_str() + self.id.as_str() } pub fn set_id(&mut self, id: &str) { - self.base_mut().id = id.to_string() + self.id = id.to_string() } pub fn interface(&self) -> Option<&str> { - self.base().interface.as_deref() + self.interface.as_deref() } pub fn set_interface(&mut self, interface: &str) { - self.base_mut().interface = Some(interface.to_string()) + self.interface = Some(interface.to_string()) } - /// A port's controller name, e.g.: bond0, br0 + /// Ports controller name, e.g.: bond0, br0 pub fn controller(&self) -> Option { - self.base().controller + self.controller } - /// Sets the port's controller. + /// Sets the ports controller. /// - /// `controller`: Uuid of the controller (Bridge, Bond, Team), e.g.: bond0. + /// `controller`: Name of the controller (Bridge, Bond, Team), e.g.: bond0. pub fn set_controller(&mut self, controller: Uuid) { - self.base_mut().controller = Some(controller) + self.controller = Some(controller) } pub fn unset_controller(&mut self) { - self.base_mut().controller = None; + self.controller = None; } pub fn uuid(&self) -> Uuid { - self.base().uuid + self.uuid } + /// FIXME: rename to ip_config pub fn ip_config(&self) -> &IpConfig { - &self.base().ip_config + &self.ip_config } pub fn ip_config_mut(&mut self) -> &mut IpConfig { - &mut self.base_mut().ip_config + &mut self.ip_config } pub fn match_config(&self) -> &MatchConfig { - &self.base().match_config + &self.match_config } pub fn match_config_mut(&mut self) -> &mut MatchConfig { - &mut self.base_mut().match_config + &mut self.match_config } pub fn remove(&mut self) { - self.base_mut().status = Status::Removed; + self.status = Status::Removed; } pub fn is_removed(&self) -> bool { - self.base().status == Status::Removed + self.status == Status::Removed } pub fn is_up(&self) -> bool { - self.base().status == Status::Up + self.status == Status::Up } pub fn set_up(&mut self) { - self.base_mut().status = Status::Up + self.status = Status::Up } pub fn set_down(&mut self) { - self.base_mut().status = Status::Down + self.status = Status::Down } /// Determines whether it is a loopback interface. pub fn is_loopback(&self) -> bool { - matches!(self, Connection::Loopback(_)) + matches!(self.config, ConnectionConfig::Loopback) } pub fn is_ethernet(&self) -> bool { - matches!(self, Connection::Loopback(_)) - || matches!(self, Connection::Ethernet(_)) - || matches!(self, Connection::Dummy(_)) - || matches!(self, Connection::Bond(_)) + matches!(self.config, ConnectionConfig::Loopback) + || matches!(self.config, ConnectionConfig::Ethernet) + || matches!(self.config, ConnectionConfig::Dummy) + || matches!(self.config, ConnectionConfig::Bond(_)) } pub fn mac_address(&self) -> String { - self.base().mac_address.to_string() + self.mac_address.to_string() } pub fn set_mac_address(&mut self, mac_address: MacAddress) { - self.base_mut().mac_address = mac_address; + self.mac_address = mac_address; } } -#[derive(Debug, Default, Clone)] -pub struct BaseConnection { - pub id: String, - pub uuid: Uuid, - pub mac_address: MacAddress, - pub ip_config: IpConfig, - pub status: Status, - pub interface: Option, - pub controller: Option, - pub match_config: MatchConfig, -} - -impl PartialEq for BaseConnection { - fn eq(&self, other: &Self) -> bool { - self.id == other.id && self.uuid == other.uuid && self.ip_config == other.ip_config - } +#[derive(Default, Debug, PartialEq, Clone)] +pub enum ConnectionConfig { + #[default] + Ethernet, + Wireless(WirelessConfig), + Loopback, + Dummy, + Bond(BondConfig), } #[derive(Debug, Error)] @@ -689,82 +648,6 @@ impl From<&IpRoute> for HashMap<&str, Value<'_>> { } } -#[derive(Debug, Default, PartialEq, Clone)] -pub struct EthernetConnection { - pub base: BaseConnection, -} - -#[derive(Debug, Default, PartialEq, Clone)] -pub struct WirelessConnection { - pub base: BaseConnection, - pub wireless: WirelessConfig, -} - -#[derive(Debug, Default, PartialEq, Clone)] -pub struct LoopbackConnection { - pub base: BaseConnection, -} - -#[derive(Debug, Default, PartialEq, Clone)] -pub struct DummyConnection { - pub base: BaseConnection, -} - -#[derive(Debug, Default, PartialEq, Clone)] -pub struct BondConnection { - pub base: BaseConnection, - pub bond: BondConfig, -} - -impl BondConnection { - pub fn set_mode(&mut self, mode: BondMode) { - self.bond.mode = mode; - } - - pub fn set_options(&mut self, options: BondOptions) { - self.bond.options = options; - } -} - -#[derive(Debug, Default, Clone, PartialEq)] -pub struct BondOptions(pub HashMap); - -impl TryFrom<&str> for BondOptions { - type Error = NetworkStateError; - - fn try_from(value: &str) -> Result { - let mut options = HashMap::new(); - - for opt in value.split_whitespace() { - let (key, value) = opt - .trim() - .split_once('=') - .ok_or(NetworkStateError::InvalidBondOptions)?; - options.insert(key.to_string(), value.to_string()); - } - - Ok(BondOptions(options)) - } -} - -impl fmt::Display for BondOptions { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let opts = &self - .0 - .iter() - .map(|(key, value)| format!("{key}={value}")) - .collect::>(); - - write!(f, "{}", opts.join(" ")) - } -} - -#[derive(Debug, Default, PartialEq, Clone)] -pub struct BondConfig { - pub mode: BondMode, - pub options: BondOptions, -} - #[derive(Debug, Default, PartialEq, Clone)] pub struct WirelessConfig { pub mode: WirelessMode, @@ -856,3 +739,42 @@ impl TryFrom<&str> for SecurityProtocol { } } } + +#[derive(Debug, Default, Clone, PartialEq)] +pub struct BondOptions(pub HashMap); + +impl TryFrom<&str> for BondOptions { + type Error = NetworkStateError; + + fn try_from(value: &str) -> Result { + let mut options = HashMap::new(); + + for opt in value.split_whitespace() { + let (key, value) = opt + .trim() + .split_once('=') + .ok_or(NetworkStateError::InvalidBondOptions)?; + options.insert(key.to_string(), value.to_string()); + } + + Ok(BondOptions(options)) + } +} + +impl fmt::Display for BondOptions { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let opts = &self + .0 + .iter() + .map(|(key, value)| format!("{key}={value}")) + .collect::>(); + + write!(f, "{}", opts.join(" ")) + } +} + +#[derive(Debug, Default, PartialEq, Clone)] +pub struct BondConfig { + pub mode: BondMode, + pub options: BondOptions, +} diff --git a/rust/agama-dbus-server/src/network/nm/dbus.rs b/rust/agama-dbus-server/src/network/nm/dbus.rs index de16523628..48736df175 100644 --- a/rust/agama-dbus-server/src/network/nm/dbus.rs +++ b/rust/agama-dbus-server/src/network/nm/dbus.rs @@ -54,22 +54,20 @@ pub fn connection_to_dbus<'a>( result.insert(ETHERNET_KEY, ethernet_config); } - match &conn { - Connection::Wireless(wireless) => { + match &conn.config { + ConnectionConfig::Wireless(wireless) => { connection_dbus.insert("type", WIRELESS_KEY.into()); - let wireless_dbus = wireless_config_to_dbus(wireless); - for (k, v) in wireless_dbus { - result.insert(k, v); - } + let wireless_dbus = wireless_config_to_dbus(wireless, &conn.mac_address); + result.extend(wireless_dbus); } - Connection::Bond(bond) => { + ConnectionConfig::Bond(bond) => { connection_dbus.insert("type", BOND_KEY.into()); if !connection_dbus.contains_key("interface-name") { connection_dbus.insert("interface-name", conn.id().into()); } result.insert("bond", bond_config_to_dbus(bond)); } - Connection::Dummy(_) => { + ConnectionConfig::Dummy => { connection_dbus.insert("type", DUMMY_KEY.into()); } _ => {} @@ -83,32 +81,30 @@ pub fn connection_to_dbus<'a>( /// /// This functions tries to turn a OwnedHashMap coming from D-Bus into a Connection. pub fn connection_from_dbus(conn: OwnedNestedHash) -> Option { - let base = base_connection_from_dbus(&conn)?; + let mut connection = base_connection_from_dbus(&conn)?; if let Some(wireless_config) = wireless_config_from_dbus(&conn) { - return Some(Connection::Wireless(WirelessConnection { - base, - wireless: wireless_config, - })); + connection.config = ConnectionConfig::Wireless(wireless_config); + return Some(connection); } if let Some(bond_config) = bond_config_from_dbus(&conn) { - return Some(Connection::Bond(BondConnection { - base, - bond: bond_config, - })); + connection.config = ConnectionConfig::Bond(bond_config); + return Some(connection); } if conn.get(DUMMY_KEY).is_some() { - return Some(Connection::Dummy(DummyConnection { base })); + connection.config = ConnectionConfig::Dummy; + return Some(connection); }; if conn.get(LOOPBACK_KEY).is_some() { - return Some(Connection::Loopback(LoopbackConnection { base })); + connection.config = ConnectionConfig::Loopback; + return Some(connection); }; if conn.get(ETHERNET_KEY).is_some() { - return Some(Connection::Ethernet(EthernetConnection { base })); + return Some(connection); }; None @@ -274,15 +270,14 @@ fn ip_config_to_ipv6_dbus(ip_config: &IpConfig) -> HashMap<&str, zvariant::Value ipv6_dbus } -fn wireless_config_to_dbus(conn: &WirelessConnection) -> NestedHash { - let config = &conn.wireless; +fn wireless_config_to_dbus<'a>( + config: &'a WirelessConfig, + mac_address: &MacAddress, +) -> NestedHash<'a> { let wireless: HashMap<&str, zvariant::Value> = HashMap::from([ ("mode", Value::new(config.mode.to_string())), ("ssid", Value::new(config.ssid.to_vec())), - ( - "assigned-mac-address", - Value::new(conn.base.mac_address.to_string()), - ), + ("assigned-mac-address", Value::new(mac_address.to_string())), ]); let mut security: HashMap<&str, zvariant::Value> = @@ -295,12 +290,9 @@ fn wireless_config_to_dbus(conn: &WirelessConnection) -> NestedHash { NestedHash::from([(WIRELESS_KEY, wireless), (WIRELESS_SECURITY_KEY, security)]) } -fn bond_config_to_dbus(conn: &BondConnection) -> HashMap<&str, zvariant::Value> { - let config = &conn.bond; - +fn bond_config_to_dbus(config: &BondConfig) -> HashMap<&str, zvariant::Value> { let mut options = config.options.0.clone(); options.insert("mode".to_string(), config.mode.to_string()); - HashMap::from([("options", Value::new(options))]) } @@ -324,7 +316,7 @@ fn match_config_to_dbus(match_config: &MatchConfig) -> HashMap<&str, zvariant::V ]) } -fn base_connection_from_dbus(conn: &OwnedNestedHash) -> Option { +fn base_connection_from_dbus(conn: &OwnedNestedHash) -> Option { let Some(connection) = conn.get("connection") else { return None; }; @@ -332,8 +324,7 @@ fn base_connection_from_dbus(conn: &OwnedNestedHash) -> Option { let id: &str = connection.get("id")?.downcast_ref()?; let uuid: &str = connection.get("uuid")?.downcast_ref()?; let uuid: Uuid = uuid.try_into().ok()?; - - let mut base_connection = BaseConnection { + let mut base_connection = Connection { id: id.to_string(), uuid, ..Default::default() @@ -761,11 +752,11 @@ mod test { let connection = connection_from_dbus(dbus_conn).unwrap(); assert_eq!(connection.mac_address(), "13:45:67:89:AB:CD".to_string()); - assert!(matches!(connection, Connection::Wireless(_))); - if let Connection::Wireless(connection) = connection { - assert_eq!(connection.wireless.ssid, SSID(vec![97, 103, 97, 109, 97])); - assert_eq!(connection.wireless.mode, WirelessMode::Infra); - assert_eq!(connection.wireless.security, SecurityProtocol::WPA2) + assert!(matches!(connection.config, ConnectionConfig::Wireless(_))); + if let ConnectionConfig::Wireless(wireless) = &connection.config { + assert_eq!(wireless.ssid, SSID(vec![97, 103, 97, 109, 97])); + assert_eq!(wireless.mode, WirelessMode::Infra); + assert_eq!(wireless.security, SecurityProtocol::WPA2) } } @@ -805,12 +796,8 @@ mod test { ssid: SSID(vec![97, 103, 97, 109, 97]), ..Default::default() }; - let wireless = WirelessConnection { - base: build_base_connection(), - wireless: config, - ..Default::default() - }; - let wireless = Connection::Wireless(wireless); + let mut wireless = build_base_connection(); + wireless.config = ConnectionConfig::Wireless(config); let wireless_dbus = connection_to_dbus(&wireless, None); let wireless = wireless_dbus.get(WIRELESS_KEY).unwrap(); @@ -838,7 +825,7 @@ mod test { #[test] fn test_dbus_from_ethernet_connection() { - let ethernet = build_ethernet_connection(); + let ethernet = build_base_connection(); let ethernet_dbus = connection_to_dbus(ðernet, None); check_dbus_base_connection(ðernet_dbus); } @@ -887,17 +874,12 @@ mod test { original.insert("ipv4".to_string(), ipv4); original.insert("ipv6".to_string(), ipv6); - let base = BaseConnection { + let ethernet = Connection { id: "agama".to_string(), interface: Some("eth0".to_string()), ..Default::default() }; - let ethernet = EthernetConnection { - base, - ..Default::default() - }; - let updated = Connection::Ethernet(ethernet); - let updated = connection_to_dbus(&updated, None); + let updated = connection_to_dbus(ðernet, None); let merged = merge_dbus_connections(&original, &updated); let connection = merged.get("connection").unwrap(); @@ -954,7 +936,7 @@ mod test { original.insert("connection".to_string(), connection); original.insert(ETHERNET_KEY.to_string(), ethernet); - let mut updated = Connection::Ethernet(EthernetConnection::default()); + let mut updated = Connection::default(); updated.set_interface(""); updated.set_mac_address(MacAddress::Unset); let updated = connection_to_dbus(&updated, None); @@ -976,7 +958,7 @@ mod test { ]) } - fn build_base_connection() -> BaseConnection { + fn build_base_connection() -> Connection { let addresses = vec![ "192.168.0.2/24".parse().unwrap(), "::ffff:c0a8:2".parse().unwrap(), @@ -998,7 +980,7 @@ mod test { ..Default::default() }; let mac_address = MacAddress::from_str("FD:CB:A9:87:65:43").unwrap(); - BaseConnection { + Connection { id: "agama".to_string(), ip_config, mac_address, @@ -1006,13 +988,6 @@ mod test { } } - fn build_ethernet_connection() -> Connection { - let ethernet = EthernetConnection { - base: build_base_connection(), - }; - Connection::Ethernet(ethernet) - } - fn check_dbus_base_connection(conn_dbus: &NestedHash) { let connection_dbus = conn_dbus.get("connection").unwrap(); let id: &str = connection_dbus.get("id").unwrap().downcast_ref().unwrap(); From 15087be5e4fb7b1abecd34e2ace3e21f614501fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Sun, 10 Dec 2023 22:16:11 +0000 Subject: [PATCH 2/2] Drop getters/setters from Connection --- .../src/network/dbus/interfaces.rs | 43 +++---- .../src/network/dbus/interfaces/ip_config.rs | 4 +- .../src/network/dbus/tree.rs | 10 +- rust/agama-dbus-server/src/network/model.rs | 117 ++++++------------ .../src/network/model/builder.rs | 2 +- .../src/network/nm/adapter.rs | 9 +- .../src/network/nm/client.rs | 14 +-- rust/agama-dbus-server/src/network/nm/dbus.rs | 50 ++++---- rust/agama-dbus-server/src/network/system.rs | 2 +- 9 files changed, 106 insertions(+), 145 deletions(-) diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces.rs b/rust/agama-dbus-server/src/network/dbus/interfaces.rs index cdce1cc67a..3cd5fb8e3d 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces.rs @@ -229,7 +229,7 @@ impl Connection { /// now), it uses the original ID but appending a number in case the ID is duplicated. #[dbus_interface(property)] pub async fn id(&self) -> String { - self.get_connection().await.id().to_string() + self.get_connection().await.id.to_string() } /// Connection UUID. @@ -238,13 +238,13 @@ impl Connection { /// backend. #[dbus_interface(property)] pub async fn uuid(&self) -> String { - self.get_connection().await.uuid().to_string() + self.get_connection().await.uuid.to_string() } #[dbus_interface(property)] pub async fn controller(&self) -> String { let connection = self.get_connection().await; - match connection.controller() { + match connection.controller { Some(uuid) => uuid.to_string(), None => "".to_string(), } @@ -252,30 +252,27 @@ impl Connection { #[dbus_interface(property)] pub async fn interface(&self) -> String { - self.get_connection() - .await - .interface() - .unwrap_or("") - .to_string() + let connection = self.get_connection().await; + connection.interface.as_deref().unwrap_or("").to_string() } #[dbus_interface(property)] pub async fn set_interface(&mut self, name: &str) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - connection.set_interface(name); + connection.interface = Some(name.to_string()); self.update_connection(connection).await } /// Custom mac-address #[dbus_interface(property)] pub async fn mac_address(&self) -> String { - self.get_connection().await.mac_address() + self.get_connection().await.mac_address.to_string() } #[dbus_interface(property)] pub async fn set_mac_address(&mut self, mac_address: &str) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - connection.set_mac_address(MacAddress::from_str(mac_address)?); + connection.mac_address = MacAddress::from_str(mac_address)?; self.update_connection(connection).await } } @@ -344,7 +341,9 @@ impl Bond { let actions = self.actions.lock().await; let (tx, rx) = oneshot::channel(); actions.send(Action::GetConnection(self.uuid, tx)).unwrap(); - rx.await.unwrap().ok_or(NetworkStateError::UnknownConnection(self.uuid.to_string())) + rx.await + .unwrap() + .ok_or(NetworkStateError::UnknownConnection(self.uuid.to_string())) } /// Gets the bonding configuration. @@ -438,14 +437,13 @@ impl Match { #[dbus_interface(property)] pub async fn driver(&self) -> Vec { let connection = self.get_connection().await; - connection.match_config().driver.clone() + connection.match_config.driver.clone() } #[dbus_interface(property)] pub async fn set_driver(&mut self, driver: Vec) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - let config = connection.match_config_mut(); - config.driver = driver; + connection.match_config.driver = driver; self.update_connection(connection).await } @@ -453,28 +451,26 @@ impl Match { #[dbus_interface(property)] pub async fn path(&self) -> Vec { let connection = self.get_connection().await; - connection.match_config().path.clone() + connection.match_config.path.clone() } #[dbus_interface(property)] pub async fn set_path(&mut self, path: Vec) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - let config = connection.match_config_mut(); - config.path = path; + connection.match_config.path = path; self.update_connection(connection).await } /// List of interface names to match. #[dbus_interface(property)] pub async fn interface(&self) -> Vec { let connection = self.get_connection().await; - connection.match_config().interface.clone() + connection.match_config.interface.clone() } #[dbus_interface(property)] pub async fn set_interface(&mut self, interface: Vec) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - let config = connection.match_config_mut(); - config.interface = interface; + connection.match_config.interface = interface; self.update_connection(connection).await } @@ -482,14 +478,13 @@ impl Match { #[dbus_interface(property)] pub async fn kernel(&self) -> Vec { let connection = self.get_connection().await; - connection.match_config().kernel.clone() + connection.match_config.kernel.clone() } #[dbus_interface(property)] pub async fn set_kernel(&mut self, kernel: Vec) -> zbus::fdo::Result<()> { let mut connection = self.get_connection().await; - let config = connection.match_config_mut(); - config.kernel = kernel; + connection.match_config.kernel = kernel; self.update_connection(connection).await } } diff --git a/rust/agama-dbus-server/src/network/dbus/interfaces/ip_config.rs b/rust/agama-dbus-server/src/network/dbus/interfaces/ip_config.rs index fbda5d3e2c..a0051c75c6 100644 --- a/rust/agama-dbus-server/src/network/dbus/interfaces/ip_config.rs +++ b/rust/agama-dbus-server/src/network/dbus/interfaces/ip_config.rs @@ -58,7 +58,7 @@ impl Ip { impl Ip { /// Returns the IpConfig struct. async fn get_ip_config(&self) -> MappedMutexGuard { - MutexGuard::map(self.get_connection().await, |c| c.ip_config_mut()) + MutexGuard::map(self.get_connection().await, |c| &mut c.ip_config) } /// Updates the IpConfig struct. @@ -69,7 +69,7 @@ impl Ip { F: Fn(&mut IpConfig), { let mut connection = self.get_connection().await; - func(connection.ip_config_mut()); + func(&mut connection.ip_config); self.update_connection(connection).await?; Ok(()) } diff --git a/rust/agama-dbus-server/src/network/dbus/tree.rs b/rust/agama-dbus-server/src/network/dbus/tree.rs index 9d2f3d3b97..45568fcf42 100644 --- a/rust/agama-dbus-server/src/network/dbus/tree.rs +++ b/rust/agama-dbus-server/src/network/dbus/tree.rs @@ -87,11 +87,11 @@ impl Tree { ) -> Result<(), ServiceError> { let mut objects = self.objects.lock().await; - let orig_id = conn.id().to_owned(); - let uuid = conn.uuid(); + let orig_id = conn.id.to_owned(); + let uuid = conn.uuid; let (id, path) = objects.register_connection(conn); - if id != conn.id() { - conn.set_id(&id) + if id != conn.id { + conn.id = id.clone(); } log::info!("Publishing network connection '{}'", id); @@ -252,7 +252,7 @@ impl ObjectsRegistry { pub fn register_connection(&mut self, conn: &Connection) -> (String, ObjectPath) { let path = format!("{}/{}", CONNECTIONS_PATH, self.connections.len()); let path = ObjectPath::try_from(path).unwrap(); - let mut id = conn.id().to_string(); + let mut id = conn.id.clone(); if self.connection_path(&id).is_some() { id = self.propose_id(&id); }; diff --git a/rust/agama-dbus-server/src/network/model.rs b/rust/agama-dbus-server/src/network/model.rs index e4c0ce64e7..e37237f381 100644 --- a/rust/agama-dbus-server/src/network/model.rs +++ b/rust/agama-dbus-server/src/network/model.rs @@ -45,7 +45,7 @@ impl NetworkState { /// /// * `id`: connection UUID pub fn get_connection_by_uuid(&self, uuid: Uuid) -> Option<&Connection> { - self.connections.iter().find(|c| c.uuid() == uuid) + self.connections.iter().find(|c| c.uuid == uuid) } /// Get connection by interface @@ -53,28 +53,30 @@ impl NetworkState { /// * `name`: connection interface name pub fn get_connection_by_interface(&self, name: &str) -> Option<&Connection> { let interface = Some(name); - self.connections.iter().find(|c| c.interface() == interface) + self.connections + .iter() + .find(|c| c.interface.as_deref() == interface) } /// Get connection by ID /// /// * `id`: connection ID pub fn get_connection(&self, id: &str) -> Option<&Connection> { - self.connections.iter().find(|c| c.id() == id) + self.connections.iter().find(|c| c.id == id) } /// Get connection by ID as mutable /// /// * `id`: connection ID pub fn get_connection_mut(&mut self, id: &str) -> Option<&mut Connection> { - self.connections.iter_mut().find(|c| c.id() == id) + self.connections.iter_mut().find(|c| c.id == id) } pub fn get_controlled_by(&mut self, uuid: Uuid) -> Vec<&Connection> { let uuid = Some(uuid); self.connections .iter() - .filter(|c| c.controller() == uuid) + .filter(|c| c.controller == uuid) .collect() } @@ -82,8 +84,8 @@ impl NetworkState { /// /// It uses the `id` to decide whether the connection already exists. pub fn add_connection(&mut self, conn: Connection) -> Result<(), NetworkStateError> { - if self.get_connection(conn.id()).is_some() { - return Err(NetworkStateError::ConnectionExists(conn.uuid())); + if self.get_connection(&conn.id).is_some() { + return Err(NetworkStateError::ConnectionExists(conn.uuid)); } self.connections.push(conn); @@ -96,8 +98,8 @@ impl NetworkState { /// /// Additionally, it registers the connection to be removed when the changes are applied. pub fn update_connection(&mut self, conn: Connection) -> Result<(), NetworkStateError> { - let Some(old_conn) = self.get_connection_mut(conn.id()) else { - return Err(NetworkStateError::UnknownConnection(conn.id().to_string())); + let Some(old_conn) = self.get_connection_mut(&conn.id) else { + return Err(NetworkStateError::UnknownConnection(conn.id.clone())); }; *old_conn = conn; @@ -134,20 +136,20 @@ impl NetworkState { .get_connection_by_interface(&port) .or_else(|| self.get_connection(&port)) .ok_or(NetworkStateError::UnknownConnection(port))?; - controlled.push(connection.uuid()); + controlled.push(connection.uuid); } for conn in self.connections.iter_mut() { - if controlled.contains(&conn.uuid()) { - conn.set_controller(controller.uuid()); - } else if conn.controller() == Some(controller.uuid()) { - conn.unset_controller(); + if controlled.contains(&conn.uuid) { + conn.controller = Some(controller.uuid); + } else if conn.controller == Some(controller.uuid) { + conn.controller = None; } } Ok(()) } else { Err(NetworkStateError::NotControllerConnection( - controller.id().to_owned(), + controller.id.to_owned(), )) } } @@ -170,7 +172,7 @@ mod tests { }; state.add_connection(conn0).unwrap(); let found = state.get_connection("eth0").unwrap(); - assert_eq!(found.uuid(), uuid); + assert_eq!(found.uuid, uuid); } #[test] @@ -201,7 +203,7 @@ mod tests { }; state.update_connection(conn1).unwrap(); let found = state.get_connection("eth0").unwrap(); - assert_eq!(found.uuid(), uuid); + assert_eq!(found.uuid, uuid); } #[test] @@ -266,9 +268,9 @@ mod tests { state.set_ports(&bond0, vec!["eth1".to_string()]).unwrap(); let eth1_found = state.get_connection("eth1").unwrap(); - assert_eq!(eth1_found.controller(), Some(bond0.uuid())); + assert_eq!(eth1_found.controller, Some(bond0.uuid)); let eth0_found = state.get_connection("eth0").unwrap(); - assert_eq!(eth0_found.controller(), None); + assert_eq!(eth0_found.controller, None); } #[test] @@ -315,8 +317,8 @@ pub struct Device { pub type_: DeviceType, } -/// Represents an available network connection -#[derive(Debug, Default, Clone)] +/// Represents an availble network connection. +#[derive(Debug, Clone)] pub struct Connection { pub id: String, pub uuid: Uuid, @@ -351,59 +353,6 @@ impl Connection { } } - pub fn id(&self) -> &str { - self.id.as_str() - } - - pub fn set_id(&mut self, id: &str) { - self.id = id.to_string() - } - - pub fn interface(&self) -> Option<&str> { - self.interface.as_deref() - } - - pub fn set_interface(&mut self, interface: &str) { - self.interface = Some(interface.to_string()) - } - - /// Ports controller name, e.g.: bond0, br0 - pub fn controller(&self) -> Option { - self.controller - } - - /// Sets the ports controller. - /// - /// `controller`: Name of the controller (Bridge, Bond, Team), e.g.: bond0. - pub fn set_controller(&mut self, controller: Uuid) { - self.controller = Some(controller) - } - - pub fn unset_controller(&mut self) { - self.controller = None; - } - - pub fn uuid(&self) -> Uuid { - self.uuid - } - - /// FIXME: rename to ip_config - pub fn ip_config(&self) -> &IpConfig { - &self.ip_config - } - - pub fn ip_config_mut(&mut self) -> &mut IpConfig { - &mut self.ip_config - } - - pub fn match_config(&self) -> &MatchConfig { - &self.match_config - } - - pub fn match_config_mut(&mut self) -> &mut MatchConfig { - &mut self.match_config - } - pub fn remove(&mut self) { self.status = Status::Removed; } @@ -435,13 +384,21 @@ impl Connection { || matches!(self.config, ConnectionConfig::Dummy) || matches!(self.config, ConnectionConfig::Bond(_)) } +} - pub fn mac_address(&self) -> String { - self.mac_address.to_string() - } - - pub fn set_mac_address(&mut self, mac_address: MacAddress) { - self.mac_address = mac_address; +impl Default for Connection { + fn default() -> Self { + Self { + id: Default::default(), + uuid: Uuid::new_v4(), + mac_address: Default::default(), + ip_config: Default::default(), + status: Default::default(), + interface: Default::default(), + controller: Default::default(), + match_config: Default::default(), + config: Default::default(), + } } } diff --git a/rust/agama-dbus-server/src/network/model/builder.rs b/rust/agama-dbus-server/src/network/model/builder.rs index 758dd52be6..8a531ce07b 100644 --- a/rust/agama-dbus-server/src/network/model/builder.rs +++ b/rust/agama-dbus-server/src/network/model/builder.rs @@ -40,7 +40,7 @@ impl ConnectionBuilder { } if let Some(controller) = self.controller { - conn.set_controller(controller); + conn.controller = Some(controller) } conn diff --git a/rust/agama-dbus-server/src/network/nm/adapter.rs b/rust/agama-dbus-server/src/network/nm/adapter.rs index 26a7f75b1e..6aa590fb0e 100644 --- a/rust/agama-dbus-server/src/network/nm/adapter.rs +++ b/rust/agama-dbus-server/src/network/nm/adapter.rs @@ -55,17 +55,18 @@ impl<'a> Adapter for NetworkManagerAdapter<'a> { if !Self::is_writable(conn) { continue; } + let result = if conn.is_removed() { - self.client.remove_connection(conn.uuid()).await + self.client.remove_connection(conn.uuid).await } else { let ctrl = conn - .controller() + .controller .and_then(|uuid| network.get_connection_by_uuid(uuid)); self.client.add_or_update_connection(conn, ctrl).await }; if let Err(e) = result { - log::error!("Could not process the connection {}: {}", conn.id(), e); + log::error!("Could not process the connection {}: {}", conn.id, e); } } }) @@ -91,7 +92,7 @@ fn add_ordered_connections<'b>( network: &'b NetworkState, conns: &mut Vec<&'b Connection>, ) { - if let Some(uuid) = conn.controller() { + if let Some(uuid) = conn.controller { let controller = network.get_connection_by_uuid(uuid).unwrap(); add_ordered_connections(controller, network, conns); } diff --git a/rust/agama-dbus-server/src/network/nm/client.rs b/rust/agama-dbus-server/src/network/nm/client.rs index 26cd0511d6..9f3a5990ed 100644 --- a/rust/agama-dbus-server/src/network/nm/client.rs +++ b/rust/agama-dbus-server/src/network/nm/client.rs @@ -88,27 +88,27 @@ impl<'a> NetworkManagerClient<'a> { if let Some(connection) = connection_from_dbus(settings.clone()) { if let Some(controller) = controller_from_dbus(&settings) { - controlled_by.insert(connection.uuid(), controller.to_string()); + controlled_by.insert(connection.uuid, controller.to_string()); } - if let Some(iname) = connection.interface() { - uuids_map.insert(iname.to_string(), connection.uuid()); + if let Some(iname) = &connection.interface { + uuids_map.insert(iname.to_string(), connection.uuid); } connections.push(connection); } } for conn in connections.iter_mut() { - let Some(interface_name) = controlled_by.get(&conn.uuid()) else { + let Some(interface_name) = controlled_by.get(&conn.uuid) else { continue; }; if let Some(uuid) = uuids_map.get(interface_name) { - conn.set_controller(*uuid); + conn.controller = Some(*uuid); } else { log::warn!( "Could not found a connection for the interface '{}' (required by connection '{}')", interface_name, - conn.id() + conn.id ); } } @@ -126,7 +126,7 @@ impl<'a> NetworkManagerClient<'a> { ) -> Result<(), ServiceError> { let mut new_conn = connection_to_dbus(conn, controller); - let path = if let Ok(proxy) = self.get_connection_proxy(conn.uuid()).await { + let path = if let Ok(proxy) = self.get_connection_proxy(conn.uuid).await { let original = proxy.get_settings().await?; let merged = merge_dbus_connections(&original, &new_conn); proxy.update(merged).await?; diff --git a/rust/agama-dbus-server/src/network/nm/dbus.rs b/rust/agama-dbus-server/src/network/nm/dbus.rs index 48736df175..df3ce6cdf8 100644 --- a/rust/agama-dbus-server/src/network/nm/dbus.rs +++ b/rust/agama-dbus-server/src/network/nm/dbus.rs @@ -28,29 +28,36 @@ pub fn connection_to_dbus<'a>( controller: Option<&'a Connection>, ) -> NestedHash<'a> { let mut result = NestedHash::new(); - let mut connection_dbus = - HashMap::from([("id", conn.id().into()), ("type", ETHERNET_KEY.into())]); + let mut connection_dbus = HashMap::from([ + ("id", conn.id.as_str().into()), + ("type", ETHERNET_KEY.into()), + ]); - if let Some(interface) = &conn.interface() { + if let Some(interface) = &conn.interface { connection_dbus.insert("interface-name", interface.to_owned().into()); } if let Some(controller) = controller { connection_dbus.insert("slave-type", "bond".into()); // TODO: only 'bond' is supported - let master = controller.interface().unwrap_or(controller.id()); + let master = controller + .interface + .as_deref() + .unwrap_or(controller.id.as_str()); connection_dbus.insert("master", master.into()); } else { connection_dbus.insert("slave-type", "".into()); // TODO: only 'bond' is supported connection_dbus.insert("master", "".into()); } - result.insert("ipv4", ip_config_to_ipv4_dbus(conn.ip_config())); - result.insert("ipv6", ip_config_to_ipv6_dbus(conn.ip_config())); - result.insert("match", match_config_to_dbus(conn.match_config())); + result.insert("ipv4", ip_config_to_ipv4_dbus(&conn.ip_config)); + result.insert("ipv6", ip_config_to_ipv6_dbus(&conn.ip_config)); + result.insert("match", match_config_to_dbus(&conn.match_config)); if conn.is_ethernet() { - let ethernet_config = - HashMap::from([("assigned-mac-address", Value::new(conn.mac_address()))]); + let ethernet_config = HashMap::from([( + "assigned-mac-address", + Value::new(conn.mac_address.to_string()), + )]); result.insert(ETHERNET_KEY, ethernet_config); } @@ -63,7 +70,7 @@ pub fn connection_to_dbus<'a>( ConnectionConfig::Bond(bond) => { connection_dbus.insert("type", BOND_KEY.into()); if !connection_dbus.contains_key("interface-name") { - connection_dbus.insert("interface-name", conn.id().into()); + connection_dbus.insert("interface-name", conn.id.as_str().into()); } result.insert("bond", bond_config_to_dbus(bond)); } @@ -673,12 +680,12 @@ mod test { let connection = connection_from_dbus(dbus_conn).unwrap(); - assert_eq!(connection.id(), "eth0"); - let ip_config = connection.ip_config(); - let match_config = connection.match_config(); + assert_eq!(connection.id, "eth0"); + let ip_config = connection.ip_config; + let match_config = connection.match_config; assert_eq!(match_config.kernel, vec!["pci-0000:00:19.0"]); - assert_eq!(connection.mac_address(), "12:34:56:78:9A:BC"); + assert_eq!(connection.mac_address.to_string(), "12:34:56:78:9A:BC"); assert_eq!( ip_config.addresses, @@ -751,7 +758,7 @@ mod test { ]); let connection = connection_from_dbus(dbus_conn).unwrap(); - assert_eq!(connection.mac_address(), "13:45:67:89:AB:CD".to_string()); + assert_eq!(connection.mac_address.to_string(), "13:45:67:89:AB:CD"); assert!(matches!(connection.config, ConnectionConfig::Wireless(_))); if let ConnectionConfig::Wireless(wireless) = &connection.config { assert_eq!(wireless.ssid, SSID(vec![97, 103, 97, 109, 97])); @@ -782,9 +789,8 @@ mod test { ]); let connection = connection_from_dbus(dbus_conn).unwrap(); - assert!(matches!(connection, Connection::Bond(_))); - if let Connection::Bond(connection) = connection { - assert_eq!(connection.bond.mode, BondMode::ActiveBackup); + if let ConnectionConfig::Bond(config) = connection.config { + assert_eq!(config.mode, BondMode::ActiveBackup); } } @@ -936,9 +942,11 @@ mod test { original.insert("connection".to_string(), connection); original.insert(ETHERNET_KEY.to_string(), ethernet); - let mut updated = Connection::default(); - updated.set_interface(""); - updated.set_mac_address(MacAddress::Unset); + let updated = Connection { + interface: Some("".to_string()), + mac_address: MacAddress::Unset, + ..Default::default() + }; let updated = connection_to_dbus(&updated, None); let merged = merge_dbus_connections(&original, &updated); diff --git a/rust/agama-dbus-server/src/network/system.rs b/rust/agama-dbus-server/src/network/system.rs index fa17121d8b..4500c3be43 100644 --- a/rust/agama-dbus-server/src/network/system.rs +++ b/rust/agama-dbus-server/src/network/system.rs @@ -130,7 +130,7 @@ impl NetworkSystem { .state .get_controlled_by(uuid) .iter() - .map(|c| c.interface().unwrap_or(c.id()).to_string()) + .map(|c| c.interface.as_deref().unwrap_or(&c.id).to_string()) .collect::>(); Ok((conn, controlled))