Clean up routes module slightly #244

jD91mZM2 · 2018-06-19T14:20:19Z

Replace update with get_ref/get_mut
Rename ipv4 add_default_ipv4_route to set since it can only have one gateway
Add get_default_ipv4_route and get_default_ipv6_routes
Add remove_default_ipv4_route and clear_default_ipv6_routes

whitequark · 2018-06-19T14:27:18Z

The update method is there so that any invariants on the internal ManagedMap can be enforced. But maybe we should not expose it at all, rather have an add_route method and such.

jD91mZM2 · 2018-06-19T14:39:10Z

I think update is fine, but there's no reason to have it take a closure :)

whitequark · 2018-06-19T14:48:37Z

There is--if you just give away a &mut there is no way to maintain any internal invariants. There aren't any right now but that is likely wrong.

jD91mZM2 · 2018-06-19T14:52:30Z

What exactly do you mean by internal invariants?

whitequark · 2018-06-19T14:55:50Z

Not every route is legal. For example, no multicast addresses should be added to the route table. This isn't checked currently, but should be, and it can't be checked with get_mut.

jD91mZM2 · 2018-06-19T14:57:49Z

Aaaaaah okay, that explains it. I'll implement your add_route idea

progval · 2018-06-21T13:58:23Z

The update method is inspired from the one in ethernet.rs, so you might want to change it as well.

jD91mZM2 · 2018-06-22T06:38:35Z

There wasn't much to do in ethernet.rs unfortunately since a ManagedSlice cannot be pushed/removed to.

whitequark · 2018-06-22T07:42:10Z

src/iface/ethernet.rs

-        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>>


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.

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

This reverts commit 3d9b73b.

whitequark · 2018-07-10T03:46:27Z

src/iface/route.rs

@@ -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();


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).

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();

See the redox netstack code after this PR.

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

whitequark · 2018-07-31T11:33:59Z

src/iface/route.rs

+            Err((_cidr, _route)) => Err(Error::Exhausted)
+        }
+    }
+    /// Returns a route registered on cidr


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.

whitequark · 2018-07-31T11:34:18Z

src/iface/route.rs

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


Same as above.

whitequark · 2018-07-31T11:35:14Z

src/iface/route.rs

+    }
+    /// Delete a route.
+    ///
+    /// On success, returns the value it deleted, if any.


returns the route it deleted, if any

whitequark · 2018-07-31T11:35:23Z

src/iface/route.rs

+    /// Delete a route.
+    ///
+    /// On success, returns the value it deleted, if any.
+    pub fn del_route(&mut self, cidr: &IpCidr) -> Option<Route> {


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

whitequark · 2018-07-31T11:36:29Z

src/iface/route.rs

+    /// 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>> {


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.

whitequark · 2018-07-31T11:39:45Z

src/iface/route.rs

@@ -105,6 +141,23 @@ impl<'a> Routes<'a> {
        }
    }

+    /// Get the current default ipv6 gateways, if any


IPv6 gateways

whitequark · 2018-07-31T11:40:29Z

src/iface/route.rs

+    #[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


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

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

whitequark · 2018-07-31T11:40:38Z

src/iface/route.rs

+        self.storage.get(&cidr).into_iter()
+    }
+
+    /// Clear default ipv6 gateways.


IPv6 gateways

whitequark · 2018-07-31T11:40:51Z

src/iface/route.rs

+
+    /// Clear default ipv6 gateways.
+    ///
+    /// On success, returns the cleared routes, if any.


This function returns exactly one route.

whitequark · 2018-07-31T11:40:59Z

src/iface/route.rs

+    #[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)


This function should use self.remove_route.

jD91mZM2 · 2018-07-31T12:25:30Z

Since you previously said you don't think this is needed, I'll just close this because I'm lazy 😛. Better now than later when I put in more time into something that isn't even desired.

jD91mZM2 force-pushed the master branch from f659156 to df40c6b Compare June 19, 2018 14:36

jD91mZM2 changed the title ~~Clean up routes module~~ Clean up routes module slightly Jun 19, 2018

jD91mZM2 force-pushed the master branch from df40c6b to a739df8 Compare June 19, 2018 14:53

Clean up routes module

161c87a

jD91mZM2 force-pushed the master branch from a739df8 to 161c87a Compare June 19, 2018 15:07

Remove update function from ethernet.rs

3d9b73b

jD91mZM2 force-pushed the master branch from 3f1c1c0 to 3d9b73b Compare June 22, 2018 06:39

whitequark reviewed Jun 22, 2018

View reviewed changes

Revert "Remove update function from ethernet.rs"

00a5dc2

This reverts commit 3d9b73b.

whitequark reviewed Jul 10, 2018

View reviewed changes

whitequark requested changes Jul 31, 2018

View reviewed changes

jD91mZM2 closed this Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up routes module slightly #244

Clean up routes module slightly #244

jD91mZM2 commented Jun 19, 2018 •

edited

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

progval commented Jun 21, 2018

jD91mZM2 commented Jun 22, 2018

whitequark Jun 22, 2018

jD91mZM2 Jun 22, 2018 •

edited

whitequark Jul 10, 2018

jD91mZM2 Jul 10, 2018

jD91mZM2 Jul 10, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

jD91mZM2 Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

whitequark Jul 31, 2018

jD91mZM2 commented Jul 31, 2018

Clean up routes module slightly #244

Clean up routes module slightly #244

Conversation

jD91mZM2 commented Jun 19, 2018 • edited

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

whitequark commented Jun 19, 2018

jD91mZM2 commented Jun 19, 2018

progval commented Jun 21, 2018

jD91mZM2 commented Jun 22, 2018

Choose a reason for hiding this comment

jD91mZM2 Jun 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 commented Jul 31, 2018

jD91mZM2 commented Jun 19, 2018 •

edited

jD91mZM2 Jun 22, 2018 •

edited