Skip to content

Use slave-type instead of port-type for NM<1.46.0 #2433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
imobachgs merged 4 commits into from
Jun 6, 2025
Merged

Use slave-type instead of port-type for NM<1.46.0 #2433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89ba3ce
Use slave-type instead of port-type for NM<1.46.0
jcronenberg Jun 3, 2025
aa024d8
Add tests for dbus from bridge and bond connections
jcronenberg Jun 3, 2025
77b12ad
Update changes
jcronenberg Jun 3, 2025
daf358b
Merge branch 'master' into slave_type_nm_compat
imobachgs Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 107 additions & 3 deletions rust/agama-network/src/nm/dbus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ pub fn connection_to_dbus<'a>(
""
}
};
connection_dbus.insert("port-type", port_type.into());
if VersionReq::parse(">=1.46.0").unwrap().matches(&nm_version) {
connection_dbus.insert("port-type", port_type.into());
} else {
connection_dbus.insert("slave-type", port_type.into());
}
let master = controller
.interface
.as_deref()
Expand All @@ -85,7 +89,11 @@ pub fn connection_to_dbus<'a>(
connection_dbus.remove("autoconnect");
connection_dbus.insert("autoconnect", false.into());
} else {
connection_dbus.insert("port-type", "".into());
if VersionReq::parse(">=1.46.0").unwrap().matches(&nm_version) {
connection_dbus.insert("port-type", "".into());
} else {
connection_dbus.insert("slave-type", "".into());
}
connection_dbus.insert("master", "".into());
}
Comment on lines 91 to 98
Copy link
Contributor Author

@jcronenberg jcronenberg Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I might be missing something but isn't this nonsensical to have, because it seems to me to get cleaned up later by cleanup_dbus_connection() anyway? So should I maybe just remove this part completely?

Copy link
Contributor Author

@jcronenberg jcronenberg Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm maybe it's relevant for merging connections? Though if the empty value then gets cleaned up before it's sent to NM, NM will just keep the value as is anyway so it will also not be unset/updated.

Copy link
Contributor

@imobachgs imobachgs Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have the same impression. That value will be cleaned-up later. I guess we need to have a closer look to this.

Copy link
Contributor

@teclator teclator Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some issue when working on this exactly because of the merge and also when for example moving a port from a bond to a bridge. It was reading the port-type but setting the slave-type or something like that..

Copy link
Contributor Author

@jcronenberg jcronenberg Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so I tested it a bit, indeed the merging seems to be a problem because for whatever reason NM still seems to send slave-type over port-type to agama and then the merging gets difficult, so I have now added a bit in cleanup_dbus_connection() that prefers the port-type of agama so that it takes precedence. And also deleting the section I mentioned here does not work because then the merge wouldn't overwrite the values/cleanup_dbus_connection() wouldn't see a change. But also what I mentioned previously

Though if the empty value then gets cleaned up before it's sent to NM, NM will just keep the value as is anyway so it will also not be unset/updated.

isn't correct in this case, if nothing is sent to NM it will actually unset this property, so cleanup_dbus_connection() is working as intended.

Copy link
Contributor

@imobachgs imobachgs Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking it. Then I guess we can proceed.


Expand Down Expand Up @@ -268,6 +276,10 @@ fn is_bridge_port(conn: &NestedHash) -> bool {
if let Ok(s) = TryInto::<&str>::try_into(port_type) {
return s == "bridge";
}
} else if let Some(port_type) = connection.get("slave-type") {
if let Ok(s) = TryInto::<&str>::try_into(port_type) {
return s == "bridge";
}
}
}

Expand Down Expand Up @@ -299,7 +311,12 @@ pub fn cleanup_dbus_connection(conn: &mut NestedHash) {
connection.remove("master");
}

if connection.get("slave-type").is_some() {
// prefer port-type over slave type
if connection.get("slave-type").is_some() && connection.get("port-type").is_some() {
connection.remove("slave-type");
}

if connection.get("slave-type").is_some_and(is_empty_value) {
connection.remove("slave-type");
}

Expand Down Expand Up @@ -2369,4 +2386,91 @@ mod test {
.unwrap();
assert_eq!(send_release, 0);
}

#[test]
fn test_dbus_from_bridge_connection() {
let mut master_con = build_base_connection();
master_con.config = ConnectionConfig::Bridge(BridgeConfig::default());
let bridge_con = build_base_connection();

let bridge_dbus = connection_to_dbus(
&bridge_con,
Some(&master_con),
semver::Version::parse("1.50.0").unwrap(),
);
let connection_dbus = bridge_dbus.get("connection").unwrap();
let port_type: &str = connection_dbus
.get("port-type")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(port_type, BRIDGE_KEY);
let master: &str = connection_dbus
.get("master")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(master, bridge_con.id);
}

#[test]
fn test_dbus_from_bond_connection() {
let mut master_con = build_base_connection();
master_con.config = ConnectionConfig::Bond(BondConfig::default());
let bond_con = build_base_connection();

let bond_dbus = connection_to_dbus(
&bond_con,
Some(&master_con),
semver::Version::parse("1.50.0").unwrap(),
);
let connection_dbus = bond_dbus.get("connection").unwrap();
let port_type: &str = connection_dbus
.get("port-type")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(port_type, BOND_KEY);
let master: &str = connection_dbus
.get("master")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(master, bond_con.id);
}

#[test]
fn test_dbus_from_bridge_connection_for_different_nm_versions() {
let mut master_con = build_base_connection();
master_con.config = ConnectionConfig::Bridge(BridgeConfig::default());
let bridge_con = build_base_connection();

let bridge_dbus = connection_to_dbus(
&bridge_con,
Some(&master_con),
semver::Version::parse("1.44.0").unwrap(),
);
let connection_dbus = bridge_dbus.get("connection").unwrap();
let slave_type: &str = connection_dbus
.get("slave-type")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(slave_type, BRIDGE_KEY);
assert_eq!(connection_dbus.get("port-type"), None);

let bridge_dbus = connection_to_dbus(
&bridge_con,
Some(&master_con),
semver::Version::parse("1.46.0").unwrap(),
);
let connection_dbus = bridge_dbus.get("connection").unwrap();
let port_type: &str = connection_dbus
.get("port-type")
.unwrap()
.downcast_ref()
.unwrap();
assert_eq!(port_type, BRIDGE_KEY);
assert_eq!(connection_dbus.get("slave-type"), None);
}
}
5 changes: 5 additions & 0 deletions rust/package/agama.changes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
-------------------------------------------------------------------
Tue Jun 5 16:03:31 UTC 2025 - Jorik Cronenberg <[email protected]>

- Use slave-type instead of port-type for NM<1.46.0 (gh#agama-project/agama#2433).

-------------------------------------------------------------------
Thu Jun 5 15:42:13 UTC 2025 - Josef Reidinger <[email protected]>

Expand Down