-
Notifications
You must be signed in to change notification settings - Fork 447
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
Add support for default gateway #44
Conversation
babe43a
to
b64a43d
Compare
Sorry for polluting the discussion, but I'm interested in following (and potentially helping) your redox integration @batonius, but I can't find where you're pushing the code in your github feed. Is this work private for the moment? I tried, long ago to use smoltcp in redox but I gave up half-way through. |
I agree. The only thing I would like to do differently here is to add a |
@little-dude It's kinda messy PoC right now, so I haven't been pushing, here it is - https://github.com/batonius/netstack/tree/smolnetd . So far I've implemented the Right now it's far from reimplementing all of the |
cf62167
to
601685e
Compare
Quick comment so this doesn't sit idle: can you please add a |
Oh sorry I already wrote this comment and forgot about it! If you don't want to deal with this I can implement it myself, no big deal. |
My first thought was to implement some kind of Having said that, it's not critical for a stop-gap solution, so if you think it's ok I can add |
Yeah sure that's what I meant. The public API needs to be changed, and I change it at will anyway, we're at 0.x. |
31202ff
to
adcf30f
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice, thanks! Just a few minor naming/style changes.
src/iface/ethernet.rs
Outdated
@@ -41,24 +42,28 @@ impl<'a, 'b, 'c, DeviceT: Device + 'a> Interface<'a, 'b, 'c, DeviceT> { | |||
/// # Panics | |||
/// See the restrictions on [set_hardware_addr](#method.set_hardware_addr) | |||
/// and [set_protocol_addrs](#method.set_protocol_addrs) functions. | |||
pub fn new<DeviceMT, ArpCacheMT, ProtocolAddrsMT> | |||
pub fn new<DeviceMT, ArpCacheMT, ProtocolAddrsMT, DefaultGWMT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think MT
used to mean something but now it can be just replaced with T
everywhere here (so GatewayT
).
src/iface/ethernet.rs
Outdated
@@ -23,7 +23,8 @@ pub struct Interface<'a, 'b, 'c, DeviceT: Device + 'a> { | |||
device: Managed<'a, DeviceT>, | |||
arp_cache: Managed<'b, ArpCache>, | |||
hardware_addr: EthernetAddress, | |||
protocol_addrs: ManagedSlice<'c, IpAddress>, | |||
protocol_addrs: ManagedSlice<'c, IpCidr>, | |||
default_gw: Option<IpAddress>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the phrase "default gateway" is a remnant of the times where a single LAN segment would conceivably have more than one gateway. At this point it's an anachronism (like classful networks!), let's call this just "gateway". Also applies elsewhere in this PR.
src/iface/ethernet.rs
Outdated
} | ||
|
||
/// Set the default gateway of the interface. | ||
pub fn set_default_gateway<T: Into<Option<IpAddress>>>(&mut self, default_gw: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there's an impl Into<Option<T>> for Option<Into<T>>
or something like it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not, I've made it explicit.
src/wire/ip.rs
Outdated
|
||
impl Cidr { | ||
/// Construct a new CIDR | ||
pub fn new(addr: Address, mask_len: u8) -> Cidr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't seem very useful as the valid ranges of prefix lengths depend on the address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rephrased it so the constructors check prefix_len and can fail now.
src/wire/ip.rs
Outdated
} | ||
} | ||
|
||
/// Return the mask_len of the CIDR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix length, not mask length; the mask is as long as the IP address, but has only as many zeroes as the prefix length.
src/wire/ipv4.rs
Outdated
/// An IPv4 CIDR containing a prefix of a IPv4 subnet. | ||
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)] | ||
pub struct Cidr { | ||
pub addr: Address, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think address
sounds nicer, and see above on mask_len.
} | ||
|
||
/// Check whether the subnet described by the CIDR conains the address. | ||
pub fn contains_ip(&self, addr: Address) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just contains
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding contains_subnet
as well.
src/wire/ip.rs
Outdated
@@ -126,6 +127,55 @@ impl fmt::Display for Address { | |||
} | |||
} | |||
|
|||
/// A CIDR containing a prefix of an IP subnet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been uncomfortable calling this data structure "CIDR" for a while but I just realized what the proper name for it would be: IpSubnet
! That makes much more sense. I'm sorry for having you change everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subnets have their own semantics, for example, 192.168.1.0/24 is exactly the same subnet as 192.168.1.255/24, this makes accepting a slice of subnets as interface addresses somewhat confusing. I've changed the documentation to better reflect the fact that CIRD != subnet, but I'd prefer to keep Cidr
as the name.
src/wire/ip.rs
Outdated
} | ||
|
||
/// Return the IP address of the CIDR. | ||
pub fn addr(&self) -> Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just address
.
src/wire/ipv4.rs
Outdated
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)] | ||
pub struct Cidr { | ||
pub addr: Address, | ||
pub mask_len: u8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this field is pub
, the struct needs an is_valid()
method, which should be checked in set_protocol_addrs
.
adcf30f
to
d6f204d
Compare
/// An IPv4 CIDR containing an IPv4 address with a prefix lenght. | ||
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Default)] | ||
pub struct Cidr { | ||
address: Address, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really odd case, because so far every other data structure in wire
has all fields pub
. It's even in the module documentation. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to prevent the creation of inconsistent values, instead of using is_valid
method, since CIDRs have an invariant that needs to be enforced.
07f0431
to
0412399
Compare
Added |
7b2cc2b
to
253a14b
Compare
Cool! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you do one final rebase, splitting into two commits: introducing IpCidr, and introducing gateways?
253a14b
to
16c1c8b
Compare
Done. |
Merged manually with some changes:
|
Context: I've actually started working on the Redox network daemon based on smoltcp I was talking about back in June and so far I was able to replace L2 and L3 daemons(ethernetd and ipd), leaving L4 daemons like udpd and tcpd working on top of the new daemon.
Problem: I'm collecting a wishlist in the process, but the only problem so far is the inability to route packets destined to an IP outside a local subnet to a default gateway, making everything outside the local subnet unaccessible.
Solution: I've implemented the bare minimum of the required functional and it appears to be working. The routing is optional and disabled by default. A proper implementation would involve actual routing tables, but I propose we adopt this or other limited solution as a stop-gap for now.