Add ipv4 and ipv6 sections to SavedConnection - #534
Conversation
GitHub Actions
Error
| && let Ok(Some(prefix)) = dict.get(&"prefix") | ||
| { | ||
| let mut route = Route::new(dest, prefix); | ||
| if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next_hop") { |
There was a problem hiding this comment.
| if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next_hop") { | |
| if let Ok(Some(next_hop)) = dict.get::<_, String>(&"next-hop") { |
https://networkmanager.dev/docs/api/latest/settings-ipv4.html#:~:text=%27next%2Dhop
There was a problem hiding this comment.
A regression test for this would be good as well
| /// Ignore the automatically configured DNS servers, and only use the saved configuration. | ||
| pub ignore_auto_dns: bool, | ||
| } |
There was a problem hiding this comment.
dns is missing from here? without it the struct is incoherent
also prefer dns-data
https://networkmanager.dev/docs/api/latest/settings-ipv4.html#:~:text=dns%2Ddata
| && let Ok(Some(address)) = dict.get::<_, &str>(&"address") | ||
| && let Ok(Some(prefix)) = dict.get::<_, u32>(&"prefix") | ||
| { | ||
| if let Ok(address) = address.parse() { |
There was a problem hiding this comment.
Malformed addresses and gateways are lost here. I believe we settled on dropping them with a warn! in #524
| /// The list of DNS search domains. | ||
| pub dns_search: Vec<String>, | ||
| /// The list of IP routes. | ||
| pub route_data: Vec<Route>, |
There was a problem hiding this comment.
Let's make this typed. We ruled out sharing structs between builders and reads in #524: builders hold only what the caller set, reads arrive fully populated with NM's defaults.
IpSettings<A> is generic and Route isn't, so an IpSettings<Ipv6Addr> can hold IPv4 route strings. And next_hop() / metric() are #[must_use] builder setters that mean nothing on decoded output.dest + prefix is already your IpAddress<A>:
IpRoute<A> { dest: IpAddress<A>, next_hop: Option<A>, metric: Option<u32> }| } | ||
| } | ||
| }; | ||
| let gateway = take_str_ref(settings, "gateway").and_then(|gateway| gateway.parse().ok()); |
| InterfaceNotFound(String), | ||
|
|
||
| #[error("invalid IP address: {0}")] | ||
| AddressParse(IpAddressParseError), |
There was a problem hiding this comment.
Nit: Hmmm I don't think I understand what the point of this is, are you planning to use it in a follow up PR for this issue?ConnectionError is #[non_exhaustive], so we can add it back if a fallible path ever appears.
| /// The method by which the connection obtains its IP address for this protocol. | ||
| pub method: IpMethod, | ||
| /// The list of IP addresses. | ||
| pub address_data: Vec<IpAddress<A>>, |
There was a problem hiding this comment.
| pub address_data: Vec<IpAddress<A>>, | |
| pub addresses: Vec<IpAddress<A>>, |
? The current name leaks NM's wire keys into our public API. wdyt?
| /// The list of DNS search domains. | ||
| pub dns_search: Vec<String>, | ||
| /// The list of IP routes. | ||
| pub route_data: Vec<Route>, |
There was a problem hiding this comment.
| pub route_data: Vec<Route>, | |
| pub routes: Vec<IpRoute<A>>, |
|
|
||
| /// How the connection obtains its IP address. | ||
| #[derive(Debug, Clone)] | ||
| pub enum IpMethod { |
There was a problem hiding this comment.
| pub enum IpMethod { | |
| #[non_exhaustive] | |
| pub enum IpMethod { |
2 participants
Fixes #524, though doesn't implement the other features discussed in the issue. Adds
ipv4andipv6sections to theSavedConnectionstruct. Foraddress_dataI use a new typedIpAddress<A>type, but I reuse theRoutestruct from the builder module forroute_data, though it may be better to create a similar typed struct, asRouteuses strings fordestandnext_hop