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 2 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
18 changes: 10 additions & 8 deletions src/iface/ethernet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,16 @@ impl<'b, 'c, 'e, DeviceT> Interface<'b, 'c, 'e, DeviceT>
self.inner.ip_addrs.as_ref()
}

/// Update the IP addresses of the interface.
/// Replace the set of IP addresses.
///
/// # Panics
/// This function panics if any of the addresses are not unicast.
pub fn update_ip_addrs<F: FnOnce(&mut ManagedSlice<'c, IpCidr>)>(&mut self, f: F) {
f(&mut self.inner.ip_addrs);
InterfaceInner::check_ip_addrs(&self.inner.ip_addrs)
pub fn set_ip_addrs<S>(&mut self, ips: S)
where S: Into<ManagedSlice<'c, IpCidr>>
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of this method is to be able to update IP addresses without assigning a different ManagedSlice. It's for memory-constrained devices without an allocator.

Copy link
Contributor Author

@jD91mZM2 jD91mZM2 Jun 22, 2018

Choose a reason for hiding this comment

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

Should I make a way to set an ip based on index? Or revert the change to that file?

{
let ips = ips.into();
InterfaceInner::check_ip_addrs(&ips);
self.inner.ip_addrs = ips;
}

/// Check whether the interface has the given IP address assigned.
Expand Down Expand Up @@ -1898,10 +1901,9 @@ mod test {
let (mut iface, _) = create_loopback();
let mut new_addrs = vec![IpCidr::new(IpAddress::v6(0xfe80, 0, 0, 0, 1, 2, 0, 2), 64),
IpCidr::new(IpAddress::v6(0xfe80, 0, 0, 0, 3, 4, 0, 0xffff), 64)];
iface.update_ip_addrs(|addrs| {
new_addrs.extend(addrs.to_vec());
*addrs = From::from(new_addrs);
});
new_addrs.extend(iface.ip_addrs());
iface.set_ip_addrs(new_addrs);

assert!(iface.inner.has_solicited_node(Ipv6Address::new(0xff02, 0, 0, 0, 0, 1, 0xff00, 0x0002)));
assert!(iface.inner.has_solicited_node(Ipv6Address::new(0xff02, 0, 0, 0, 0, 1, 0xff00, 0xffff)));
assert!(!iface.inner.has_solicited_node(Ipv6Address::new(0xff02, 0, 0, 0, 0, 1, 0xff00, 0x0003)));
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