[WIP] IPv6 Address and CIDR to wire #72

dlrobertson · 2017-11-07T03:40:24Z

Add some of the basics of ipv6.

Add the ipv6 feature
- Ensure there is a travis build with the ipv6 feature enabled.
Add the basic infrastructure to wire for IPv6 support.
- Ipv6Address
- Ipv6Cidr

dlrobertson

I'll try to keep future PRs much much smaller.

dlrobertson · 2017-11-07T03:43:04Z

Cargo.toml

@@ -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"] }


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.

For your reference, you can do "ipv6" = ["byteorder/i128"].

dlrobertson · 2017-11-07T03:48:55Z

src/wire/ipv6/extops.rs

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


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

whitequark · 2017-11-07T04:03:20Z

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:

Make IPv4 optional by adding a feature gating it.
Add Ipv6Address/Ipv6Cidr.
Add Ipv6Packet/Ipv6Repr.

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.

dlrobertson · 2017-11-07T04:16:34Z

Can you please split it into many PRs, and start small?

Np

Please do not yet make any provisions for handling extension headers, multicast, etc

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.

whitequark · 2017-11-07T04:18:49Z

We can go without extension headers.

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.

dlrobertson · 2017-11-07T04:23:35Z

Gotcha. Makes way more sense now 😄 I'll try to get a super trimmed down version tomorrow.

dlrobertson · 2017-11-09T00:54:18Z

Implemented 2 first so that when we add the ipv4 feature we don't hit a ton of unused and unreachable errors

whitequark · 2017-11-09T00:57:56Z

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.

whitequark · 2017-11-09T00:58:05Z

Well, and it breaks the build.

dlrobertson · 2017-11-09T01:08:15Z

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.

Np, I'll figure out something

whitequark · 2017-11-09T01:00:48Z

src/wire/ip.rs

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


I think IPv6 addresses are typically constructed from 16-bit parts, not 8-bit parts.

whitequark · 2017-11-09T01:10:50Z

src/wire/ip.rs

+            (&Cidr::Ipv6(ref cidr), &Address::Ipv6(ref addr)) =>
+                cidr.contains_addr(addr),
+            #[cfg(feature = "ipv6")]
+            (&Cidr::Ipv4(_), &Address::Ipv6(_)) | (&Cidr::Ipv6(_), &Address::Ipv4(_)) =>


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.

Good question. Would the conversions typically happen in Repr::lower

whitequark · 2017-11-09T01:25:54Z

src/wire/ip.rs

@@ -387,6 +467,23 @@ impl Repr {
                    dst_addr, protocol, payload_len, ttl
                }))
            }
+            #[cfg(feature = "ipv6")]


... and a missing newline

whitequark · 2017-11-09T01:27:16Z

src/wire/ip.rs

+                dst_addr: Address::Ipv4(_),
+                src_addr: Address::Ipv6(_),
+                ..
+            } => panic!("mix of IP address versions"),


There's already a (commented) case below that catches this.

whitequark · 2017-11-09T01:29:25Z

src/wire/ipv6/mod.rs

+
+pub use super::IpProtocol;
+
+pub mod core;


Let's not split it into modules yet, I like the regularity of the current module tree a lot.

whitequark · 2017-11-09T01:30:30Z

src/wire/ipv6/core.rs

+
+pub use super::IpProtocol as Protocol;
+
+/// A four-octet IPv6 address


Smoltcp doc comments all have a dot at the end of the first sentence.

whitequark · 2017-11-09T01:47:02Z

src/wire/ipv6/core.rs

+    /// Return an IPv6 address as a sequence of octets, in big-endian.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }


Should also have from_parts(&[u16]) and (bikeshed the name) write_parts(&mut [u16])

whitequark · 2017-11-09T01:49:09Z

src/wire/ipv6/core.rs

+    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}",


Split this line in two please

whitequark · 2017-11-09T02:08:48Z

src/wire/ipv6/core.rs

+
+    #[test]
+    fn address_format() {
+        assert_eq!("ff02:0000:0000:0000:0000:0000:0000:0001",


This PR would also be well served if you added an IPv6 parser to it.

dlrobertson

Updated with (I think) all of your comments addressed

dlrobertson · 2017-11-11T17:54:22Z

src/parsers.rs

@@ -115,6 +119,96 @@ impl<'a> Parser<'a> {
        Err(())
    }

+    #[cfg(feature = "ipv6")]
+    fn accept_ipv6(&mut self, is_cidr: bool) -> Result<Ipv6Address> {


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.

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

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 😄

Oh I guess you can look ahead for whether there's a : and bail out if there is not, instead of looking for EOF.

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.

dlrobertson · 2017-11-11T17:54:52Z

src/parsers.rs

@@ -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)) {


Used a match because you can't add a cfg to an if.

dlrobertson · 2017-11-11T17:55:32Z

src/parsers.rs

+        }
+
+        #[cfg(feature = "ipv6")]
+        match Ipv6Cidr::from_str(s) {


Used a match so that a cfg could be used here.

whitequark · 2017-11-13T04:25:42Z

src/wire/ipv6.rs

+
+// Helper used to convert 8 given u16 arguments to a [u8; 16]
+// slice
+macro_rules! write_to_addr {


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.

Nah, just being lazy. Updated.

batonius · 2017-11-13T22:15:46Z

Should IpEndpoint have FromStr implementation as well? While it's trivial to split an IPv4 endpoint at ':', IPv6 endpoints are quite tricky: https://tools.ietf.org/html/rfc5952#section-6 . I think the recommended style would be enough, it's the only one std::net::SocketAddrV6 supports: https://doc.rust-lang.org/nightly/src/std/net/parser.rs.html#280 .

whitequark · 2017-11-13T21:33:06Z

src/parsers.rs

@@ -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]),


Nit: should be indented after (.

whitequark · 2017-11-13T21:33:56Z

src/parsers.rs

+
+        #[cfg(feature = "ipv6")]
+        match self.try(|p| p.accept_ipv6()) {
+            Some(ipv6) => { return Ok(IpAddress::Ipv6(ipv6)); },


Nit: too much visual noise. Some(ipv6) => return Ok(IpAddress::Ipv6(ipv6)), None => () is just fine.

whitequark · 2017-11-13T21:34:11Z

src/parsers.rs

+
+        #[cfg(feature = "ipv6")]
+        match Ipv6Cidr::from_str(s) {
+            Ok(cidr) => { return Ok(IpCidr::Ipv6(cidr)); },


Nit: visual noise

whitequark · 2017-11-13T21:34:27Z

src/socket/raw.rs

@@ -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) },


Nit: visual noise

whitequark · 2017-11-13T22:16:51Z

@batonius Yes.

whitequark · 2017-11-13T22:30:32Z

src/parsers.rs

+                // Tail or head section is too long
+                Err(())
+            }
+            None if self.lookahead_char(b'/') => {


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.

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

whitequark · 2017-11-13T22:32:28Z

src/parsers.rs

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


This check isn't doing anything; it would prevent a zero-length copy.

whitequark · 2017-11-13T22:32:40Z

src/parsers.rs

+        // 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]);


Why does tail have the type [u16; 6]?

It can only be 6 words in length. The double colon consumes 32 bits at a minimum.

dlrobertson · 2017-11-25T19:42:28Z

Cleaned up some logic

whitequark

One last round of changes and I'm merging this PR.

whitequark · 2017-11-27T10:33:26Z

Cargo.toml

@@ -33,6 +33,7 @@ alloc = ["managed/alloc"]
 verbose = []
 "phy-raw_socket" = ["std", "libc"]
 "phy-tap_interface" = ["std", "libc"]
+"ipv6" = []


"proto-ipv6"

whitequark · 2017-11-27T10:34:49Z

src/parsers.rs

@@ -17,6 +21,15 @@ impl<'a> Parser<'a> {
        }
    }

+    #[cfg(feature = "ipv6")]


Let's mark this entire file #[cfg_attr(not(and(feature = "proto-ipv4", feature = "proto-ipv6")), allow(dead_code)].

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

whitequark · 2017-11-27T10:36:27Z

src/parsers.rs

@@ -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]),


👍 An excellent job refactoring this method, it is very easy to see now that it is correct.

whitequark · 2017-11-27T10:44:30Z

src/wire/ip.rs

+            &Address::Unspecified     => Address::Unspecified,
+            &Address::Ipv4(_)         => Address::Ipv4(Ipv4Address::UNSPECIFIED),
+            #[cfg(feature = "ipv6")]
+            &Address::Ipv6(_)         => Address::Ipv4(Ipv4Address::UNSPECIFIED),


Copy-paste mistake.

whitequark · 2017-11-27T10:46:03Z

src/wire/ipv6.rs

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


These look ugly, perhaps add a line break after =, indent, and then lay out in two lines?

whitequark · 2017-11-27T10:47:27Z

src/wire/ipv6.rs

+    /// Construct an IPv6 address from a sequence of words, in big-endian.
+    ///
+    /// # Panics
+    /// The function panics if `data` is not 8 words long


Should panic but doesn't.

whitequark · 2017-11-27T10:49:06Z

src/wire/ipv6.rs

+
+    /// Query whether the IPv6 address is the "loopback" address
+    pub fn is_loopback(&self) -> bool {
+        self.0 == [0x00, 0x00, 0x00, 0x00,


== Self::LOOPBACK?

whitequark · 2017-11-27T10:49:35Z

src/wire/ipv6.rs

+        self.0[0] == 0xff
+    }
+
+    /// Query whether the IPv6 address is the "unspecified" address


Missing dots in doc-comments here and in many other places.

whitequark · 2017-11-27T10:51:32Z

src/wire/ipv6.rs

+               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])


This could really use a very simple :: shortener (start munging at the first 0u16 part).

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

whitequark · 2017-11-27T10:52:10Z

src/wire/ipv6.rs

+
+/// 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)]


Cidr is not PartialOrd in any meaningful way I can see.

whitequark · 2017-11-28T00:24:09Z

src/wire/ipv6.rs

+        } else {
+            let mut words = [0u16; 8];
+            self.write_parts(&mut words);
+            match words.iter().position(|x| *x == 0) {


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

?

whitequark · 2017-11-28T00:25:20Z

src/wire/ipv6.rs

+    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])


Seems weird to stringify those since we cannot parse them. It should be one or the other.

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

whitequark

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.

dlrobertson · 2017-11-28T22:34:20Z

Added some comments to the Ipv6Address Display implementation and a test case for the Ipv6Address parser.

dlrobertson mentioned this pull request Nov 7, 2017

IPv6 #3

Closed

dlrobertson commented Nov 7, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch from 3bbc760 to 5682ccf Compare November 9, 2017 00:03

dlrobertson changed the title ~~[WIP] IPv6 basics in wire~~ [WIP] IPv6 Address and CIDR to wire Nov 9, 2017

dlrobertson mentioned this pull request Nov 9, 2017

Add IPv6 to wire #76

Closed

3 tasks

whitequark requested changes Nov 9, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch 5 times, most recently from 82e67b7 to f96e199 Compare November 11, 2017 17:52

dlrobertson commented Nov 11, 2017

View reviewed changes

whitequark reviewed Nov 13, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch 4 times, most recently from f61137c to 89e2403 Compare November 13, 2017 21:23

whitequark requested changes Nov 13, 2017

View reviewed changes

whitequark reviewed Nov 13, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch from 89e2403 to 7a4fc0d Compare November 13, 2017 22:33

whitequark requested changes Nov 13, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch from 7a4fc0d to 2c8fa21 Compare November 13, 2017 22:52

dlrobertson mentioned this pull request Nov 14, 2017

IpEndpoint should have a FromStr implementation #81

Closed

dlrobertson force-pushed the ipv6 branch 3 times, most recently from 9a50b60 to 53e5009 Compare November 25, 2017 19:41

whitequark requested changes Nov 27, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch 2 times, most recently from 2b014ba to 32ef6f6 Compare November 28, 2017 00:06

whitequark reviewed Nov 28, 2017

View reviewed changes

whitequark requested changes Nov 28, 2017

View reviewed changes

dlrobertson force-pushed the ipv6 branch from 32ef6f6 to bba6193 Compare November 28, 2017 05:05

Add IPv6 address and cidr to wire

b498382

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

dlrobertson force-pushed the ipv6 branch from bba6193 to b498382 Compare November 28, 2017 22:32

whitequark approved these changes Nov 29, 2017

View reviewed changes

whitequark merged commit 7727687 into smoltcp-rs:master Nov 29, 2017

dlrobertson deleted the ipv6 branch November 29, 2017 13:29


		pub use super::IpProtocol as Protocol;

		/// A four-octet IPv6 address

[WIP] IPv6 Address and CIDR to wire #72

[WIP] IPv6 Address and CIDR to wire #72

Conversation

dlrobertson commented Nov 7, 2017 • edited

dlrobertson left a 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

whitequark commented Nov 7, 2017 • edited

dlrobertson commented Nov 7, 2017

whitequark commented Nov 7, 2017

dlrobertson commented Nov 7, 2017

dlrobertson commented Nov 9, 2017

whitequark commented Nov 9, 2017

whitequark commented Nov 9, 2017

dlrobertson commented Nov 9, 2017

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

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson Nov 13, 2017 • 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

batonius commented Nov 13, 2017

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

whitequark commented Nov 13, 2017

Choose a reason for hiding this comment

dlrobertson Nov 13, 2017 • 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

dlrobertson commented Nov 25, 2017

whitequark left a 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

Choose a reason for hiding this comment

whitequark Nov 28, 2017 • 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

whitequark left a comment

Choose a reason for hiding this comment

dlrobertson commented Nov 28, 2017

dlrobertson commented Nov 7, 2017 •

edited

whitequark commented Nov 7, 2017 •

edited

dlrobertson Nov 13, 2017 •

edited

dlrobertson Nov 13, 2017 •

edited

whitequark Nov 28, 2017 •

edited