Bonding DBUS API fixes

rust/agama-dbus-server/src/network/action.rs

@@ -1,5 +1,10 @@
+use crate::network::dbus::ControllerSettings;
 use crate::network::model::Connection;
 use agama_lib::network::types::DeviceType;
+use std::collections::HashMap;
+use tokio::sync::oneshot;
+
+use super::error::NetworkStateError;
 
 /// Networking actions, like adding, updating or removing connections.
 ///
@@ -12,7 +17,11 @@ pub enum Action {
  /// Update a connection (replacing the old one).
  UpdateConnection(Connection),
  /// Update a controller connection (replacing the old one).
- UpdateControllerConnection(Connection, Vec<String>),
+ UpdateControllerConnection(
+ Connection,
+ HashMap<String, ControllerSettings>,
+ oneshot::Sender<Result<Connection, NetworkStateError>>,
+ ),
  /// Remove the connection with the given Uuid.
  RemoveConnection(String),
  /// Apply the current configuration.

rust/agama-dbus-server/src/network/dbus.rs

@@ -7,5 +7,6 @@ mod interfaces;
 pub mod service;
 mod tree;
 
+pub use interfaces::ControllerSettings;
 pub use service::NetworkService;
 pub(crate) use tree::{ObjectsRegistry, Tree};

rust/agama-dbus-server/src/network/dbus/interfaces.rs

@@ -16,7 +16,7 @@ use crate::network::{
 
 use agama_lib::network::types::SSID;
 use std::sync::Arc;
-use tokio::sync::mpsc::UnboundedSender;
+use tokio::sync::{mpsc::UnboundedSender, oneshot};
 use tokio::sync::{MappedMutexGuard, Mutex, MutexGuard};
 use zbus::{
  dbus_interface,
@@ -288,7 +288,7 @@ impl Match {
 
 /// D-Bus interface for Bond settings
 pub struct Bond {
- actions: Arc<Mutex<Sender<Action>>>,
+ actions: Arc<Mutex<UnboundedSender<Action>>>,
  connection: Arc<Mutex<NetworkConnection>>,
 }
 
@@ -297,7 +297,10 @@ impl Bond {
  ///
  /// * `actions`: sending-half of a channel to send actions.
  /// * `connection`: connection to expose over D-Bus.
- pub fn new(actions: Sender<Action>, connection: Arc<Mutex<NetworkConnection>>) -> Self {
+ pub fn new(
+ actions: UnboundedSender<Action>,
+ connection: Arc<Mutex<NetworkConnection>>,
+ ) -> Self {
  Self {
  actions: Arc::new(Mutex::new(actions)),
  connection,
@@ -307,7 +310,7 @@ impl Bond {
  /// Gets the bond connection.
  ///
  /// Beware that it crashes when it is not a bond connection.
- async fn get_bond(&self) -> MappedMutexGuard<NetworkConnection, BondConnection> {
+ async fn get_bond(&self) -> MappedMutexGuard<BondConnection> {
  MutexGuard::map(self.connection.lock().await, |c| match c {
  NetworkConnection::Bond(config) => config,
  _ => panic!("Not a bond connection. This is most probably a bug."),
@@ -319,23 +322,22 @@ impl Bond {
  /// * `connection`: Updated connection.
  async fn update_controller_connection<'a>(
  &self,
- connection: MappedMutexGuard<'a, NetworkConnection, BondConnection>,
- settings: HashMap<String, BondSettings>,
- ) -> zbus::fdo::Result<()> {
+ connection: MappedMutexGuard<'a, BondConnection>,
+ settings: HashMap<String, ControllerSettings>,
+ ) -> Result<crate::network::model::Connection, NetworkStateError> {
  let actions = self.actions.lock().await;
  let connection = NetworkConnection::Bond(connection.clone());
+ let (tx, rx) = oneshot::channel();
  actions
- .send(Action::UpdateControllerConnection(
- connection.clone(),
- settings,
- ))
- .await
+ .send(Action::UpdateControllerConnection(connection, settings, tx))
  .unwrap();
- Ok(())
+
+ rx.await.unwrap()
  }
 }
 
-enum BondSettings {
+#[derive(Debug, PartialEq, Clone)]
+pub enum ControllerSettings {
  Ports(Vec<String>),
  Options(String),
 }
@@ -346,20 +348,24 @@ impl Bond {
  #[dbus_interface(property)]
  pub async fn options(&self) -> String {
  let connection = self.get_bond().await;
- let mut opts = vec![];
- for (key, value) in &connection.bond.options {
- opts.push(format!("{key}={value}"));
- }
 
- opts.join(" ")
+ connection.bond.options.to_string()
  }
 
  #[dbus_interface(property)]
- pub async fn set_options(&self, opts: String) -> zbus::fdo::Result<()> {
+ pub async fn set_options(&mut self, opts: String) -> zbus::fdo::Result<()> {
  let connection = self.get_bond().await;
-
- self.update_controller_connection(connection, HashMap::from(["options", opts.clone()]))
- .await
+ let result = self
+ .update_controller_connection(
+ connection,
+ HashMap::from([(
+ "options".to_string(),
+ ControllerSettings::Options(opts.clone()),
+ )]),
+ )
+ .await;
+ self.connection = Arc::new(Mutex::new(result.unwrap()));
+ Ok(())
  }
 
  /// List of bond ports.
@@ -370,15 +376,24 @@ impl Bond {
  .bond
  .ports
  .iter()
- .map(|port| port.base().interface.to_string())
+ .map(|port| port.base().id.to_string())
  .collect()
  }
 
  #[dbus_interface(property)]
  pub async fn set_ports(&mut self, ports: Vec<String>) -> zbus::fdo::Result<()> {
  let connection = self.get_bond().await;
- self.update_controller_connection(connection, HashMap::from(["ports", ports.clone()]))
- .await
+ let result = self
+ .update_controller_connection(
+ connection,
+ HashMap::from([(
+ "ports".to_string(),
+ ControllerSettings::Ports(ports.clone()),
+ )]),
+ )
+ .await;
+ self.connection = Arc::new(Mutex::new(result.unwrap()));
+ Ok(())
  }
 }
 

rust/agama-dbus-server/src/network/model.rs

@@ -16,6 +16,8 @@ use thiserror::Error;
 use uuid::Uuid;
 use zbus::zvariant::Value;
 
+use super::dbus::ControllerSettings;
+
 #[derive(Default, Clone)]
 pub struct NetworkState {
  pub devices: Vec<Device>,
@@ -96,7 +98,7 @@ impl NetworkState {
  pub fn update_controller_connection(
  &mut self,
  mut conn: Connection,
- ports: Vec<String>,
+ settings: HashMap<String, ControllerSettings>,
  ) -> Result<(), NetworkStateError> {
  // let Some(old_conn) = self.get_connection_mut(conn.id()) else {
  // return Err(NetworkStateError::UnknownConnection(conn.id().to_string()));
@@ -105,29 +107,28 @@ impl NetworkState {
  let mut new_ports = vec![];
 
  if let Connection::Bond(ref mut bond) = conn {
- // if let Connection::Bond(old_bond) = old_conn {
- // let moved_ports = old_bond.bond.ports.into_iter().filter(|p| ports.contains(&p.base().interface));
- // for port in old_bond.bond.ports.iter() {
- // if ports.contains(&port.base().interface) {
- // bond.bond.ports.push(port.clone())
- // }
- // }
- //}
-
- for port in ports.into_iter() {
- new_ports.push(
- if let Some(new_port) = bond.bond.ports.iter().find(|c| c.interface() == port) {
- new_port.clone()
- } else {
- Connection::new(port, DeviceType::Ethernet)
- },
- );
+ if let Some(ControllerSettings::Options(opts)) = settings.get("options") {
+ bond.bond.options = BondOptions::try_from(opts.clone()).unwrap()
  }
 
- bond.bond.ports = new_ports;
+ if let Some(ControllerSettings::Ports(ports)) = settings.get("ports") {
+ for port in ports.iter() {
+ new_ports.push(
+ if let Some(new_port) =
+ bond.bond.ports.iter().find(|c| c.interface() == port)
+ {
+ new_port.clone()
+ } else {
+ Connection::new(port.to_string(), DeviceType::Ethernet)
+ },
+ );
+ }
+
+ bond.bond.ports = new_ports;
+ }
  }
 
- Ok(())
+ self.update_connection(conn)
  }
 
  /// Removes a connection from the state.
@@ -549,10 +550,39 @@ pub struct BondConnection {
  pub bond: BondConfig,
 }
 
+#[derive(Debug, Default, Clone, PartialEq)]
+pub struct BondOptions(pub HashMap<String, String>);
+
+impl TryFrom<String> for BondOptions {
+ type Error = NetworkStateError;
+
+ fn try_from(value: String) -> Result<Self, Self::Error> {
+ let mut options = HashMap::new();
+
+ for opt in value.split_whitespace() {
+ let opt_word: Vec<&str> = opt.trim().split('=').collect();
+ options.insert(opt_word[0].to_string(), opt_word[1].to_string());
+ }
+
+ Ok(BondOptions(options))
+ }
+}
+
+impl fmt::Display for BondOptions {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let mut opts = vec![];
+ for (key, value) in &self.0 {
+ opts.push(format!("{key}={value}"));
+ }
+
+ write!(f, "{}", opts.join(" "))
+ }
+}
+
 #[derive(Debug, Default, PartialEq, Clone)]
 pub struct BondConfig {
  pub ports: Vec<Connection>,
- pub options: HashMap<String, String>,
+ pub options: BondOptions,
 }
 
 #[derive(Debug, Default, Clone, PartialEq)]

rust/agama-dbus-server/src/network/nm/dbus.rs

@@ -65,7 +65,6 @@ pub fn connection_from_dbus(conn: OwnedNestedHash) -> Option<Connection> {
  }
 
  if let Some(bond_config) = bond_config_from_dbus(&conn) {
- println!("Returning bond config");
  return Some(Connection::Bond(BondConnection {
  base,
  bond: bond_config,
@@ -251,7 +250,7 @@ fn wireless_config_to_dbus(conn: &WirelessConnection) -> NestedHash {
 fn bond_config_to_dbus(conn: &BondConnection) -> HashMap<&str, zvariant::Value> {
  let config = &conn.bond;
 
- HashMap::from([("options", Value::new(config.options.clone()))])
+ HashMap::from([("options", Value::new(config.options.0.clone()))])
 }
 
 /// Converts a MatchConfig struct into a HashMap that can be sent over D-Bus.
@@ -514,7 +513,7 @@ fn bond_config_from_dbus(conn: &OwnedNestedHash) -> Option<BondConfig> {
  let options = <HashMap<String, String>>::try_from(dict.clone()).unwrap();
 
  Some(BondConfig {
- options: options,
+ options: BondOptions(options),
  ..Default::default()
  })
 }

rust/agama-dbus-server/src/network/system.rs

@@ -73,8 +73,15 @@ impl<T: Adapter> NetworkSystem<T> {
  Action::UpdateConnection(conn) => {
  self.state.update_connection(conn)?;
  }
- Action::UpdateControllerConnection(conn, ports) => {
- self.state.update_controller_connection(conn, ports)?;
+ Action::UpdateControllerConnection(conn, settings, tx) => {
+ let id = &conn.clone();
+ let id = id.id();
+ self.state.update_controller_connection(conn, settings)?;
+ if let Some(conn) = self.state.get_connection(id) {
+ tx.send(Ok(conn.clone())).unwrap();
+ }
+
+ dbg!(&self.state.connections);
  }
  Action::RemoveConnection(id) => {
  self.tree.remove_connection(&id).await?;