feat: implement Linux support for connectivity detection (#5)#7
Conversation
1be0251 to
9723253
Compare
| status | ||
| } | ||
|
|
||
| fn map_connectivity(connectivity: u32) -> ConnectedState { |
There was a problem hiding this comment.
Should NM_CONNECTIVITY_PORTAL map to Connected with constrained: true, rather than Disconnected? I think this is similar to the question @jhafer raised in the other PR.
There was a problem hiding this comment.
Agreed, updated this to report portal connectivity as connected but constrained.
| path.as_str() == "/" | ||
| } | ||
|
|
||
| fn default_route_interface(route_table: &str) -> Option<String> { |
There was a problem hiding this comment.
/proc/net/route lists every candidate route, including down ones. Should we filter on routes which are reported as being up?
There was a problem hiding this comment.
Agreed. I updated the passive fallback to require the default route to have the RTF_UP flag before treating it as connected
| | `constrained` | `boolean` | Whether the connection is data-constrained or restricted | | ||
| | `connectionType` | `ConnectionType` | The physical transport: `wifi`, `ethernet`, `cellular`, `unknown` | | ||
|
|
||
| #### Platform mapping |
There was a problem hiding this comment.
Can we also please document the platform mapping for Linux?
| NM_CONNECTIVITY_NONE | NM_CONNECTIVITY_PORTAL | NM_CONNECTIVITY_LIMITED => { | ||
| ConnectedState::Disconnected | ||
| } | ||
| NM_CONNECTIVITY_UNKNOWN => ConnectedState::Unknown, |
There was a problem hiding this comment.
Nit: NM_CONNECTIVITY_UNKNOWN => Unknown and _ => Unknown are equivalent.
There was a problem hiding this comment.
Good catch. I simplified the match so unknown and unrecognized values both use the fallback arm.
10f4dcb to
c376dad
Compare
c376dad to
70b2703
Compare
70b2703 to
0737cff
Compare
| } | ||
|
|
||
| fn is_constrained(connectivity_state: ConnectedState, metered: bool, roaming: bool) -> bool { | ||
| connectivity_state == ConnectedState::Constrained || metered || roaming |
There was a problem hiding this comment.
On Linux, metered: true always forces constrained: true, whereas the Windows backend derives the two from separate signals. Could you confirm that's the desired behavior?
There was a problem hiding this comment.
Yes, this is intentional for Linux. NetworkManager does not expose a separate background-data policy signal, so we treat an explicitly or guessed metered primary device as constrained. I added a code comment to make that mapping explicit.
| .build() | ||
| } | ||
|
|
||
| fn service_has_owner(connection: &Connection, service: &str) -> std::result::Result<bool, String> { |
There was a problem hiding this comment.
This returns Result<bool, String>, while the rest of the module returns zbus::Result. Should this be updated?
There was a problem hiding this comment.
Agreed. I updated service_has_owner() to return zbus::Result<bool> and propagate the zbus/name errors directly.
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| struct PrimaryConnectionDetails { |
There was a problem hiding this comment.
This is field-for-field identical to DeviceDetails. Can these be unified into a single type?
There was a problem hiding this comment.
I originally kept them separate because they represented different semantic levels: per-device details vs aggregated primary-connection details. Since the fields and behavior are identical, unifying them is cleaner. Updated to use a single ConnectionDetails type.
0737cff to
c245698
Compare
| windows = { version = "0.62.2", features = ["Networking_Connectivity"] } | ||
|
|
||
| [target.'cfg(target_os = "linux")'.dependencies] | ||
| zbus = "=5.14.0" |
There was a problem hiding this comment.
Since this crate is a published library, downstream consumers ignore our committed Cargo.lock, so an exact =5.14.0 requirement becomes a hard constraint in their dependency tree — it can cause version-unification conflicts if another crate wants zbus 5.15, and it opts us out of patch-level (incl. security) fixes for a dependency that parses system-bus IPC. The rest of the manifest uses caret ranges (windows, tracing). Was the exact pin deliberate (e.g. guarding against zbus's history of intra-5.x churn)? If so, a ~5.14 (allow 5.14.x patches, block 5.15) plus a one-line comment explaining the intent would keep the conservatism without the downstream friction. MSRV looks fine either way — zbus 5.14 and its tree resolve well below the stated 1.94.0.
Comment added by AI model Claude
There was a problem hiding this comment.
Updated to zbus = "5.16.0" and refreshed Cargo.lock.
|
|
||
| fn system_bus_connection() -> zbus::Result<Connection> { | ||
| ConnectionBuilder::system()? | ||
| .method_timeout(DBUS_TIMEOUT) |
There was a problem hiding this comment.
Minor scope note: method_timeout bounds each individual method call on the resulting connection, but it does not bound the socket connect + mandatory Hello handshake that build() performs here, nor is it an overall budget for connection_status(). Because CacheProperties::No forces a fresh round-trip per property, a connected-modem-on-metered-roaming path issues ~8-10 sequential calls, each able to hit the 2s ceiling — so the worst case under a wedged NetworkManager is closer to 16-20s than 2s. The spawn_blocking wrapper in commands.rs keeps this off the UI thread (good), so this is about request latency, not a UI hang. Worth a short comment documenting that 2s is per-method, and possibly an overall wall-clock budget before giving up on NM and using the passive fallback.
Comment added by AI model Claude
There was a problem hiding this comment.
Good point. I renamed the constant to DBUS_METHOD_TIMEOUT and added a comment clarifying that it applies per D-Bus method call, not to the initial connection handshake or the full status query. I did not add an overall wall-clock timeout here because the current blocking zbus path cannot be cleanly cancelled mid-call. If needed, we can handle that in a larger async/cancellable design change, or by accepting that timed-out work may keep running in the background.
| details.roaming |= device_details.roaming; | ||
|
|
||
| if details.connection_type == ConnectionType::Unknown { | ||
| details.connection_type = device_details.connection_type.clone(); |
There was a problem hiding this comment.
ConnectionType is a fieldless enum that already derives Clone/PartialEq/Eq, so it qualifies for Copy. Adding Copy to its derive in src/types.rs is non-breaking and lets this .clone() compile down to a plain move — the explicit clone currently reads as if the type carries heap data when it doesn't.
Comment added by AI model Claude
There was a problem hiding this comment.
Good call. I added Copy to ConnectionType and removed the explicit .clone() from the Linux aggregation path.
| } | ||
| } | ||
|
|
||
| if !read_any_device { |
There was a problem hiding this comment.
read_any_device is only ever used to gate this debug line — it never affects the returned value, so it reads as a code path that was left unfinished. More importantly, the case it detects is the one worth handling: when the primary connection has devices but every device read fails, this returns ConnectionDetails::default() → metered: false. Combined with the per-device Metered read also defaulting to false on error, a transient D-Bus hiccup on a genuinely metered/cellular link is reported as metered: false, which could lead a consumer to use capped data. Would it be safer to fail conservative here — e.g. when no device could be read on a known-active connection, treat metered as true (or surface Unknown) rather than false? At minimum, either act on read_any_device or remove it.
Comment added by AI model Claude
There was a problem hiding this comment.
Agreed. I changed the unresolved-device paths to fail conservative: if the primary connection details or all device reads fail, we now treat the active connection as metered. I also changed per-device Metered read failures to bias metered and added a test for that fallback.
| match connectivity { | ||
| NM_CONNECTIVITY_FULL => ConnectedState::Connected, | ||
| NM_CONNECTIVITY_PORTAL => ConnectedState::Constrained, | ||
| NM_CONNECTIVITY_NONE | NM_CONNECTIVITY_LIMITED => ConnectedState::Disconnected, |
There was a problem hiding this comment.
A cross-path consistency question on the most-consumed field, connected: here LIMITED (route present, NM's connectivity probe failed) maps to Disconnected → connected: false, while fallback_connection_status() reports connected: true for any up, non-loopback default route with no reachability check at all. So the same machine on the same no-internet-but-routed network flips connected depending on whether NetworkManager happens to be installed (e.g. desktop Ubuntu vs. a minimal container / WSL2). NM's LIMITED is semantically what the fallback observes (link up, internet unverified) — it just can't run the probe. Could these be reconciled, e.g. LIMITED -> Constrained (connected+constrained, mirroring how PORTAL is handled) so both paths agree that a routed-but-unverified link is "connected but don't trust it"? Not necessarily blocking, but worth a deliberate decision since connected is the field meant to be platform-stable.
Comment added by AI model Claude
There was a problem hiding this comment.
Agreed. I changed LIMITED to map to connected + constrained so it lines up with the passive fallback’s routed-link semantics. Updated the tests and docs to make that mapping explicit.
| } | ||
| }; | ||
|
|
||
| let Some(iface) = default_route_interface(&route_table) else { |
There was a problem hiding this comment.
The passive fallback reads only /proc/net/route, which is IPv4-only. IPv6 default routes live in /proc/net/ipv6_route (different, header-less hex format). On an IPv6-only network — increasingly common (some cellular carriers, certain cloud/CI environments) — a host with a perfectly good ::/0 default route would report connected: false from this path. Worth also checking /proc/net/ipv6_route for a default route before concluding disconnected.
Comment added by AI model Claude
There was a problem hiding this comment.
Good catch. I added IPv6 default-route parsing to the passive fallback before returning disconnected, with tests for valid, loopback, down, missing, and malformed IPv6 route rows. Also updated the docs to mention the IPv4/IPv6 fallback behavior.
| fn infer_transport_from_sysfs(sys_class_net: &Path, iface: &str) -> ConnectionType { | ||
| let interface_path = sys_class_net.join(iface); | ||
|
|
||
| if interface_path.join("wireless").exists() || has_child_path_marker(&interface_path, "80211") { |
There was a problem hiding this comment.
The Wi-Fi sysfs detection substring-matches "80211" against every component of the canonicalized path (path_has_component does .to_ascii_lowercase().contains(marker)). Since /sys/class/net/<iface> canonicalizes into the full /sys/devices/.../net/<iface> real path, any ancestor component that merely contains the digits 80211 (or, for the wwan marker, the substring wwan) would false-positive. The authoritative kernel markers are the phy80211 symlink (or ieee80211 dir) for cfg80211/mac80211 devices; wireless is the legacy WEXT marker that isn't guaranteed on modern drivers. Matching the directory name exactly — e.g. interface_path.join("phy80211").exists() alongside the existing wireless check — would be both more correct and simpler (drops the recursive read_dir scan). The current test passes under both the substring and exact-match logic, so a negative test (a path fragment containing 80211 that must not classify as Wi-Fi) would guard the fix.
Comment added by AI model Claude
There was a problem hiding this comment.
Good catch. I replaced the substring-based sysfs scan with exact marker checks for Wi-Fi and tightened the WWAN subsystem check to exact component matching. Added negative tests for 80211/wwan path-fragment false positives too.
|
@gabref @velocitysystems Claude added some comments for consideration. |
Linux needs a passive backend that works with NetworkManager when available. Smaller systems without NetworkManager should still get a predictable answer. Use cached NetworkManager properties for connectivity, metering, and transport. Fall back to the kernel route table and sysfs instead of active probes.
Update the Linux support matrix and API docs to describe how the backend maps NetworkManager connectivity, portal state, metering, transport, and constrained fallback behavior.
Manual Linux testing is easier when the example app emits plugin debug logs. Install a debug-friendly tracing subscriber in the example app. Refresh its lockfile for logging and the Linux backend dependency graph.
Document tested Linux scenarios, expected responses, and setup commands. Note that ModemManager cellular and roaming scenarios remain untested.
c245698 to
fe9b56a
Compare
Adds Linux support for
connection_status()intauri-plugin-connectivity./proc/net/routeand sysfs when NetworkManager is unavailableCloses #5