-
Notifications
You must be signed in to change notification settings - Fork 446
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
[WIP] IPv6 Address and CIDR to wire #72
Conversation
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'll try to keep future PRs much much smaller.
Cargo.toml
Outdated
@@ -12,7 +12,7 @@ categories = ["embedded"] | |||
license = "0BSD" | |||
|
|||
[dependencies] | |||
byteorder = { version = "1.0", default-features = false } | |||
byteorder = { version = "1.0", default-features = false, features = ["i128"] } |
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 couldn't find any docs on a way to enable a dependency feature based on a feature enabled in the parent project when the dependency isn't optional. If there is a way to conditionally enable this feature based on the ipv6
feature. We should definitely do that.
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.
For your reference, you can do "ipv6" = ["byteorder/i128"]
.
src/wire/ipv6/extops.rs
Outdated
|
||
impl<'a, T: AsRef<[u8]> + AsMut<[u8]> + ?Sized> Packet<&'a mut T> { | ||
/// Return a mutable pointer to the payload. | ||
pub fn payload_mut(&mut self) -> &mut [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.
Using this on a Pad1
option is more or less a user error since it cannot have a payload. I went back and forth on using an assert(self.opt_type() != ExtOpType::Pad1)
.
Okay, this is way too much code to even discuss, much less merge, in a single PR. Can you please split it into many PRs, and start small? I suggest the following:
Please do not yet make any provisions for handling extension headers, multicast, etc, I would like to first see a simple, clean version of the IPv6 basics go in. Then we can think about those superfluous things. |
Np
We can go without extension headers. We will not be able to get to the next header for any packet that contains an option, but even if we find out we need to implement options, we might be able to get away with implementing the absolute basics and not actually parsing the options contained within the given header. We can delay implementing multicast, but we'll need multicast to implement address resolution. |
To elaborate: I'm not saying that smoltcp doesn't need extension headers or multicast. I'm saying that I want to have a good view of the core functionality before I start building on it, so that I don't go overboard with complexity. |
Gotcha. Makes way more sense now 😄 I'll try to get a super trimmed down version tomorrow. |
Implemented 2 first so that when we add the ipv4 feature we don't hit a ton of unused and unreachable errors |
I would strongly prefer that you do not use the i128 type. Not only it requires a feature, but also it is a code generation hazard on the kinds of targets smoltcp aims to be suitable for. E.g. the OR1K backend we use at M-Labs crashes trying to codegen it. |
Well, and it breaks the build. |
Np, I'll figure out something |
src/wire/ip.rs
Outdated
@@ -75,38 +98,56 @@ impl Address { | |||
Address::Ipv4(Ipv4Address::new(a0, a1, a2, a3)) | |||
} | |||
|
|||
/// Create an address wrapping an IPv6 address with the given octets. | |||
#[cfg(feature = "ipv6")] | |||
pub fn v6(a0: u8, a1: u8, a2: u8, a3: 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.
I think IPv6 addresses are typically constructed from 16-bit parts, not 8-bit parts.
src/wire/ip.rs
Outdated
(&Cidr::Ipv6(ref cidr), &Address::Ipv6(ref addr)) => | ||
cidr.contains_addr(addr), | ||
#[cfg(feature = "ipv6")] | ||
(&Cidr::Ipv4(_), &Address::Ipv6(_)) | (&Cidr::Ipv6(_), &Address::Ipv4(_)) => |
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 wonder what should we do with IPv6-mapped IPv4 addresses. I think the conversion should happen on a layer above this, though, since IPv6-mapped IPv4 addresses shouldn't really ever appear on wire.
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.
Good question. Would the conversions typically happen in Repr::lower
src/wire/ip.rs
Outdated
@@ -387,6 +467,23 @@ impl Repr { | |||
dst_addr, protocol, payload_len, ttl | |||
})) | |||
} | |||
#[cfg(feature = "ipv6")] |
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.
... and a missing newline
src/wire/ip.rs
Outdated
dst_addr: Address::Ipv4(_), | ||
src_addr: Address::Ipv6(_), | ||
.. | ||
} => panic!("mix of IP address versions"), |
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.
There's already a (commented) case below that catches this.
src/wire/ipv6/mod.rs
Outdated
|
||
pub use super::IpProtocol; | ||
|
||
pub mod core; |
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.
Let's not split it into modules yet, I like the regularity of the current module tree a lot.
src/wire/ipv6/core.rs
Outdated
|
||
pub use super::IpProtocol as Protocol; | ||
|
||
/// A four-octet IPv6 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.
Smoltcp doc comments all have a dot at the end of the first sentence.
src/wire/ipv6/core.rs
Outdated
/// Return an IPv6 address as a sequence of octets, in big-endian. | ||
pub fn as_bytes(&self) -> &[u8] { | ||
&self.0 | ||
} |
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.
Should also have from_parts(&[u16])
and (bikeshed the name) write_parts(&mut [u16])
src/wire/ipv6/core.rs
Outdated
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
let bytes = self.0; | ||
write!(f, | ||
"{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}:{:02x}{:02x}", |
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.
Split this line in two please
src/wire/ipv6/core.rs
Outdated
|
||
#[test] | ||
fn address_format() { | ||
assert_eq!("ff02:0000:0000:0000:0000:0000:0000:0001", |
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 PR would also be well served if you added an IPv6 parser to it.
82e67b7
to
f96e199
Compare
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.
Updated with (I think) all of your comments addressed
src/parsers.rs
Outdated
@@ -115,6 +119,96 @@ impl<'a> Parser<'a> { | |||
Err(()) | |||
} | |||
|
|||
#[cfg(feature = "ipv6")] | |||
fn accept_ipv6(&mut self, is_cidr: bool) -> Result<Ipv6Address> { |
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 tried to simplify this as much as possible, but since it is of a variable length, parsing is a bit complex. Suggestions on methods to make this more readable are very much welcomed.
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 easiest way to make this parser much simpler would be to make it recursive. Here's an outline:
fn accept_ipv6_part(&mut self,
(head, tail): (&mut [u16; 8], &mut [u16; 8]),
(head_idx, tail_idx): (&mut usize, &mut usize),
mut use_tail: bool) -> Result<()> {
match self.accept_str(b"::") {
Ok(()) => use_tail = true,
Err(()) => ()
}
if head_idx != 0 || use_tail && tail_idx != 0 {
self.accept_char(b':')?;
}
match self.accept_number(4, 0x10000, true)? {
Ok(part) if use_tail => {
tail[tail_idx] = part;
tail_idx += 1;
}
Ok(part) => {
head[head_idx] = part;
head_idx += 1;
}
Err(()) => ()
}
if head_idx + tail_idx > 8 {
Err(())
} else if self.pos == self.data.len() {
head[8 - tail.len()..].copy_from_slice(tail);
Ok(())
} else {
self.accept_ipv6_part((head, tail), (head_idx, tail_idx), use_tail)
}
}
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.
Can we rely on (self.pos == self.data.len()) == true
event for a CIDR? I thought for the current implementation I just had to hand processing over as soon as I hit a non-number in the second parsing round (if is_cidr
is true) or when I filled the addr buffer. Either way, adding that to the above would be pretty easy and this is def more readable. I'll work on this tomorrow after I get some sleep. I need more brain power to think through this 😄
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.
Oh I guess you can look ahead for whether there's a :
and bail out if there is not, instead of looking for EOF.
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.
Updated to use the recursive method and used lookahead_char
instead of is_cidr
. IMO it makes it less confusing and there are less mutable variables you need to keep track of.
src/parsers.rs
Outdated
@@ -130,6 +224,13 @@ impl<'a> Parser<'a> { | |||
if let Some(ipv4) = self.try(|p| p.accept_ipv4()) { | |||
return Ok(IpAddress::Ipv4(ipv4)) | |||
} | |||
|
|||
#[cfg(feature = "ipv6")] | |||
match self.try(|p| p.accept_ipv6(false)) { |
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.
Used a match
because you can't add a cfg
to an if
.
src/parsers.rs
Outdated
} | ||
|
||
#[cfg(feature = "ipv6")] | ||
match Ipv6Cidr::from_str(s) { |
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.
Used a match
so that a cfg
could be used here.
src/wire/ipv6.rs
Outdated
|
||
// Helper used to convert 8 given u16 arguments to a [u8; 16] | ||
// slice | ||
macro_rules! write_to_addr { |
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.
Do you really need a macro for something that will be used exactly once? Just expand it with copy/paste, it's more clear how it works then.
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.
Nah, just being lazy. Updated.
f61137c
to
89e2403
Compare
Should |
src/parsers.rs
Outdated
@@ -115,6 +136,110 @@ impl<'a> Parser<'a> { | |||
Err(()) | |||
} | |||
|
|||
#[cfg(feature = "ipv6")] | |||
fn accept_ipv6_part(&mut self, | |||
(head, tail): (&mut [u16; 8], &mut [u16; 6]), |
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: should be indented after (
.
src/parsers.rs
Outdated
|
||
#[cfg(feature = "ipv6")] | ||
match self.try(|p| p.accept_ipv6()) { | ||
Some(ipv6) => { return Ok(IpAddress::Ipv6(ipv6)); }, |
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: too much visual noise. Some(ipv6) => return Ok(IpAddress::Ipv6(ipv6)), None => ()
is just fine.
src/parsers.rs
Outdated
|
||
#[cfg(feature = "ipv6")] | ||
match Ipv6Cidr::from_str(s) { | ||
Ok(cidr) => { return Ok(IpCidr::Ipv6(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.
Nit: visual noise
src/socket/raw.rs
Outdated
@@ -206,6 +206,8 @@ impl<'a, 'b> RawSocket<'a, 'b> { | |||
let ipv4_repr = Ipv4Repr::parse(&packet, checksum_caps)?; | |||
Ok((IpRepr::Ipv4(ipv4_repr), packet.payload())) | |||
} | |||
#[cfg(feature = "ipv6")] | |||
IpVersion::Ipv6 => { Err(Error::Unrecognized) }, |
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: visual noise
@batonius Yes. |
src/parsers.rs
Outdated
// Tail or head section is too long | ||
Err(()) | ||
} | ||
None if self.lookahead_char(b'/') => { |
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.
No, this is not right. You shouldn't look for any specific character. Rather, you should bail out if you detect anything you don't expect.
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 particular branch is really only applicable to CIDRs (specifically one with a double colon before the prefix e.g. fe80::/64
), so maybe I should reintroduce is_cidr
then
src/parsers.rs
Outdated
|
||
// If the tail_idx is not 0, we need to copy the tail portion | ||
// (the portion following the "::") to the end of the address. | ||
if tail_idx != 0 { |
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 check isn't doing anything; it would prevent a zero-length copy.
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.
Correct
// possible second section after the "::", and finally | ||
// combine the second optional section to the end of the | ||
// final address. | ||
let (mut addr, mut tail) = ([0u16; 8], [0u16; 6]); |
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.
Why does tail have the type [u16; 6]
?
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.
It can only be 6 words in length. The double colon consumes 32 bits at a minimum.
9a50b60
to
53e5009
Compare
Cleaned up some logic |
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.
One last round of changes and I'm merging this PR.
Cargo.toml
Outdated
@@ -33,6 +33,7 @@ alloc = ["managed/alloc"] | |||
verbose = [] | |||
"phy-raw_socket" = ["std", "libc"] | |||
"phy-tap_interface" = ["std", "libc"] | |||
"ipv6" = [] |
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.
"proto-ipv6"
src/parsers.rs
Outdated
@@ -17,6 +21,15 @@ impl<'a> Parser<'a> { | |||
} | |||
} | |||
|
|||
#[cfg(feature = "ipv6")] |
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.
Let's mark this entire file #[cfg_attr(not(and(feature = "proto-ipv4", feature = "proto-ipv6")), allow(dead_code)]
.
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.
Added #[cfg_attr(not(feature = "proto-ipv6"), allow(dead_code))]
. Will add the proto-ipv4
case once I move on to the proto-ipv4
feature component of #76
src/parsers.rs
Outdated
@@ -115,6 +136,103 @@ impl<'a> Parser<'a> { | |||
Err(()) | |||
} | |||
|
|||
#[cfg(feature = "ipv6")] | |||
fn accept_ipv6_part(&mut self, (head, tail): (&mut [u16; 8], &mut [u16; 6]), |
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.
👍 An excellent job refactoring this method, it is very easy to see now that it is correct.
src/wire/ip.rs
Outdated
&Address::Unspecified => Address::Unspecified, | ||
&Address::Ipv4(_) => Address::Ipv4(Ipv4Address::UNSPECIFIED), | ||
#[cfg(feature = "ipv6")] | ||
&Address::Ipv6(_) => Address::Ipv4(Ipv4Address::UNSPECIFIED), |
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.
Copy-paste mistake.
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.
Good catch
src/wire/ipv6.rs
Outdated
pub const UNSPECIFIED: Address = Address([0x00; 16]); | ||
|
||
/// Link local all routers multicast address | ||
pub const LINK_LOCAL_ALL_NODES: Address = Address([0xff, 0x02, 0x00, 0x0, |
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.
These look ugly, perhaps add a line break after =
, indent, and then lay out in two lines?
src/wire/ipv6.rs
Outdated
/// Construct an IPv6 address from a sequence of words, in big-endian. | ||
/// | ||
/// # Panics | ||
/// The function panics if `data` is not 8 words long |
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.
Should panic but doesn'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.
Good catch
src/wire/ipv6.rs
Outdated
|
||
/// Query whether the IPv6 address is the "loopback" address | ||
pub fn is_loopback(&self) -> bool { | ||
self.0 == [0x00, 0x00, 0x00, 0x00, |
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.
== Self::LOOPBACK
?
src/wire/ipv6.rs
Outdated
self.0[0] == 0xff | ||
} | ||
|
||
/// Query whether the IPv6 address is the "unspecified" 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.
Missing dots in doc-comments here and in many other places.
src/wire/ipv6.rs
Outdated
bytes[0], bytes[1], bytes[2], bytes[3], | ||
bytes[4], bytes[5], bytes[6], bytes[7], | ||
bytes[8], bytes[9], bytes[10], bytes[11], | ||
bytes[12], bytes[13], bytes[14], bytes[15]) |
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 could really use a very simple ::
shortener (start munging at the first 0u16
part).
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 took a very simple approach and added the IPv4 mapped IPv6 address case. Ipv4 Compatible addresses have been deprecated, so there is no need to add a case for them. I thought about essentially copy and pasting https://github.com/rust-lang/rust/blob/1.19.0/src/libstd/net/ip.rs#L1113-L1182, but slice pattern matching is used
src/wire/ipv6.rs
Outdated
|
||
/// A specification of an IPv6 CIDR block, containing an address and a variable-length | ||
/// subnet masking prefix length. | ||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Default)] |
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.
Cidr
is not PartialOrd
in any meaningful way I can see.
2b014ba
to
32ef6f6
Compare
src/wire/ipv6.rs
Outdated
} else { | ||
let mut words = [0u16; 8]; | ||
self.write_parts(&mut words); | ||
match words.iter().position(|x| *x == 0) { |
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.
Maybe...
enum State { Head, Body, Tail }
let mut state = Head;
for word in words {
state = match (word, state) {
(0, Head) |
(0, Body) => { write!(f, "::"); Tail },
(_, Head) => { write!(f, "{:04x}", word); Body },
(_, Body) |
(_, Tail) => { write!(f, ":{:04x}", word); state }
}
}
?
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.
Nice!
src/wire/ipv6.rs
Outdated
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
if self.0[0..10] == [0u8; 10] && self.0[10..12] == [0xffu8; 2] { | ||
// The address is a IPv4 mapped address | ||
write!(f, "::ffff:{}.{}.{}.{}", self.0[12], self.0[13], self.0[14], self.0[15]) |
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.
Seems weird to stringify those since we cannot parse them. It should be one or the other.
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.
After thinking about this again, adding IPv4 mapped addresses could be a good follow up PR. AFAIK From<Ipv4Address>
should be implemented for Ipv6Addr
, we should be able to parse IPv4 mapped addresses, and we should print them differently. I'll remove this for now
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.
A few more things to do with formatting...
- Add the ipv6 feature - Ensure a travis build with the ipv6 feature enabled. - Add the necessary infrastructure to wire for ipv6 support. - Ipv6Address - Ipv6Cidr - Add Ipv6 Address and Cidr parsing to parsers - Add basic tests.
Added some comments to the |
Add some of the basics of
ipv6
.ipv6
featureipv6
feature enabled.Ipv6Address
Ipv6Cidr