Skip to content
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.

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

Clean up routes module slightly #244

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn main() {
let default_v4_gw = Ipv4Address::new(192, 168, 69, 100);
let mut routes_storage = [None; 1];
let mut routes = Routes::new(&mut routes_storage[..]);
routes.add_default_ipv4_route(default_v4_gw).unwrap();
routes.set_default_ipv4_route(default_v4_gw).unwrap();
let mut iface = EthernetInterfaceBuilder::new(device)
.ethernet_addr(ethernet_addr)
.neighbor_cache(neighbor_cache)
Expand Down
2 changes: 1 addition & 1 deletion examples/httpclient.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ fn main() {
let default_v6_gw = Ipv6Address::new(0xfe80, 0, 0, 0, 0, 0, 0, 0x100);
let mut routes_storage = [None; 2];
let mut routes = Routes::new(&mut routes_storage[..]);
routes.add_default_ipv4_route(default_v4_gw).unwrap();
routes.set_default_ipv4_route(default_v4_gw).unwrap();
routes.add_default_ipv6_route(default_v6_gw).unwrap();
let mut iface = EthernetInterfaceBuilder::new(device)
.ethernet_addr(ethernet_addr)
Expand Down
2 changes: 1 addition & 1 deletion examples/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ fn main() {
let default_v6_gw = Ipv6Address::new(0xfe80, 0, 0, 0, 0, 0, 0, 0x100);
let mut routes_storage = [None; 2];
let mut routes = Routes::new(&mut routes_storage[..]);
routes.add_default_ipv4_route(default_v4_gw).unwrap();
routes.set_default_ipv4_route(default_v4_gw).unwrap();
routes.add_default_ipv6_route(default_v6_gw).unwrap();
let mut iface = EthernetInterfaceBuilder::new(device)
.ethernet_addr(ethernet_addr)
Expand Down
71 changes: 60 additions & 11 deletions src/iface/route.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,36 @@ impl<'a> Routes<'a> {
Routes { storage }
}

/// Update the routes of this node.
pub fn update<F: FnOnce(&mut ManagedMap<'a, IpCidr, Route>)>(&mut self, f: F) {
f(&mut self.storage);
/// Add a new route.
///
/// On success, return the previous route, if any.
pub fn add_route(&mut self, cidr: IpCidr, route: Route) -> Result<Option<Route>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of this function is inconsistent with set_*_default_route. Either this should be called set_route, or those functions should be called add_*_default_route. They fundamentally do the same thing.

// TODO: Validate
match self.storage.insert(cidr, route) {
Ok(route) => Ok(route),
Err((_cidr, _route)) => Err(Error::Exhausted)
}
}
/// Returns a route registered on cidr
Copy link
Contributor

Choose a reason for hiding this comment

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

This docstring isn't informative at all. What does "registered on cidr" mean? It's not clear from the documentation whether this function would only return a route that is an exact match or any route matching the argument.

Also, "CIDR" should be capitalized, it's an acronym.

Also, there's a missing newline.

pub fn get_route(&mut self, cidr: &IpCidr) -> Option<&Route> {
self.storage.get(cidr)
}
/// Returns a mutable reference to the route registered by cidr
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

pub fn get_route_mut(&mut self, cidr: &IpCidr) -> Option<&mut Route> {
self.storage.get_mut(cidr)
}
/// Delete a route.
///
/// On success, returns the value it deleted, if any.
Copy link
Contributor

Choose a reason for hiding this comment

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

returns the route it deleted, if any

pub fn del_route(&mut self, cidr: &IpCidr) -> Option<Route> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should be called delete_route (or remove_route; not del_route).

self.storage.remove(&cidr)
}

/// Add a default ipv4 gateway (ie. "ip route add 0.0.0.0/0 via `gateway`").
/// Set the default ipv4 gateway (ie. "ip route add 0.0.0.0/0 via `gateway`").
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv4 gateway

///
/// On success, returns the previous default route, if any.
#[cfg(feature = "proto-ipv4")]
pub fn add_default_ipv4_route(&mut self, gateway: Ipv4Address) -> Result<Option<Route>> {
pub fn set_default_ipv4_route(&mut self, gateway: Ipv4Address) -> Result<Option<Route>> {
let cidr = IpCidr::new(IpAddress::v4(0, 0, 0, 0), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should use self.add_route.

let route = Route::new_ipv4_gateway(gateway);
match self.storage.insert(cidr, route) {
Expand All @@ -92,6 +112,22 @@ impl<'a> Routes<'a> {
}
}

/// Get the current default ipv4 gateway, if any
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv4 gateway

#[cfg(feature = "proto-ipv4")]
pub fn get_default_ipv4_route(&mut self) -> Option<&Route> {
let cidr = IpCidr::new(IpAddress::v4(0, 0, 0, 0), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should use self.get_route.

self.storage.get(&cidr)
}

/// Remove default ipv4 gateway.
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv4 gateway.

///
/// Returns the removed route, if any.
#[cfg(feature = "proto-ipv4")]
pub fn remove_default_ipv4_route(&mut self) -> Option<Route> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again, inconsistent naming with del_route.

let cidr = IpCidr::new(IpAddress::v4(0, 0, 0, 0), 0);
self.storage.remove(&cidr)
}

/// Add a default ipv6 gateway (ie. "ip -6 route add ::/0 via `gateway`").
///
/// On success, returns the previous default route, if any.
Expand All @@ -105,6 +141,23 @@ impl<'a> Routes<'a> {
}
}

/// Get the current default ipv6 gateways, if any
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv6 gateways

#[cfg(feature = "proto-ipv6")]
pub fn get_default_ipv6_routes(&mut self) -> impl Iterator<Item = &Route> {
let cidr = IpCidr::new(IpAddress::v6(0, 0, 0, 0, 0, 0, 0, 0), 0);
// TODO: Support multiple default ipv6 gateways
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a TODO if you already return an iterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I return an iterator over one thing, but I heard that in the future you might support multiple

self.storage.get(&cidr).into_iter()
}

/// Clear default ipv6 gateways.
Copy link
Contributor

Choose a reason for hiding this comment

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

IPv6 gateways

///
/// On success, returns the cleared routes, if any.
Copy link
Contributor

Choose a reason for hiding this comment

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

This function returns exactly one route.

#[cfg(feature = "proto-ipv6")]
pub fn clear_default_ipv6_routes(&mut self) -> Option<Route> {
let cidr = IpCidr::new(IpAddress::v6(0, 0, 0, 0, 0, 0, 0, 0), 0);
self.storage.remove(&cidr)
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should use self.remove_route.

}

pub(crate) fn lookup(&self, addr: &IpAddress, timestamp: Instant) ->
Option<IpAddress> {
assert!(addr.is_unicast());
Expand Down Expand Up @@ -195,9 +248,7 @@ mod test {
via_router: ADDR_1A.into(),
preferred_until: None, expires_at: None,
};
routes.update(|storage| {
storage.insert(cidr_1().into(), route).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

So what does this achieve? One line less in route setup code? I'm not convinced it's worth adding so many functions in the public API (and you don't even validate input in some of them).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, imagine you want to fetch the default gateway with the current code:

let mut gateway = None;
routes.update(|storage| {
     let cidr = IpCidr::new(IpAddress::v4(0, 0, 0, 0), 0);
    gateway = Some(storage.get(&cidr).clone());
});
let gateway = gateway.unwrap();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the redox netstack code after this PR.

(Your existing code doesn't validate input either btw)

});
routes.add_route(cidr_1().into(), route).unwrap();

assert_eq!(routes.lookup(&ADDR_1A.into(), Instant::from_millis(0)), Some(ADDR_1A.into()));
assert_eq!(routes.lookup(&ADDR_1B.into(), Instant::from_millis(0)), Some(ADDR_1A.into()));
Expand All @@ -210,9 +261,7 @@ mod test {
preferred_until: Some(Instant::from_millis(10)),
expires_at: Some(Instant::from_millis(10)),
};
routes.update(|storage| {
storage.insert(cidr_2().into(), route2).unwrap();
});
routes.add_route(cidr_2().into(), route2).unwrap();

assert_eq!(routes.lookup(&ADDR_1A.into(), Instant::from_millis(0)), Some(ADDR_1A.into()));
assert_eq!(routes.lookup(&ADDR_1B.into(), Instant::from_millis(0)), Some(ADDR_1A.into()));
Expand Down