Add the proto-ipv4 feature #98

dlrobertson · 2017-12-22T02:01:42Z

Users should be able to choose to use smoltcp without IPv4 support.

Fixes: #76

whitequark · 2017-12-22T06:56:49Z

src/socket/tcp.rs

@@ -1488,6 +1488,7 @@ impl<'a> fmt::Write for TcpSocket<'a> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


Let's add pub(crate) const MOCK_IP_ADDRESS_1 in smoltcp::wire::ip::test or something like this, it's not good to drop tests in IPv6...

After looking at the tests, I'm not sure how a set of mock IP addresses would work. So I added the ipv4 specific stuff in a ipv4_locals module. Would the public mock IpAddress be an IPv6 variant if proto-ipv6 was enabled and IPv4 if not? Or would we have a public collection of mock IPv4 addresses and a public collection of IPv6 addresses.

Would the public mock IpAddress be an IPv6 variant if proto-ipv6 was enabled and IPv4 if not?

Yes

whitequark · 2017-12-22T06:57:03Z

src/socket/udp.rs

@@ -257,6 +257,7 @@ impl<'a, 'b> UdpSocket<'a, 'b> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


As mentioned elsewhere.

whitequark · 2017-12-22T07:00:23Z

src/wire/tcp.rs

@@ -853,6 +853,7 @@ impl<T: AsRef<[u8]>> PrettyPrint for Packet<T> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


As mentioned elsewhere.

whitequark · 2017-12-22T07:00:50Z

src/wire/udp.rs

@@ -281,6 +282,7 @@ impl<T: AsRef<[u8]>> PrettyPrint for Packet<T> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


As mentioned elsewhere.

whitequark · 2017-12-22T07:01:34Z

src/iface/ethernet.rs

@@ -21,7 +25,7 @@ use wire::{TcpPacket, TcpRepr, TcpControl};
 use socket::{Socket, SocketSet, AnySocket};
 #[cfg(feature = "socket-raw")]
 use socket::RawSocket;
-#[cfg(feature = "socket-icmp")]
+#[cfg(all(feature = "socket-icmp", feature = "proto-ipv4"))]


I don't think ICMP sockets should depend on proto-ipv4. Sure, right now, since we don't have ICMPv6 at all, they would do nothing, but they should eventually be available, so let's keep them in.

If we remove this that would mean a build without default features and without proto-ipv4 but with proto-ipv6 would fail. If you're okay with that, I can simply remove this. Otherwise we need to make a generic IcmpRepr/IcmpPacket

Hmmm, I've looked at making ICMP sockets work with ICMPv6 but it looks involved, so let's disable them on non-IPv4 builds for now.

Sounds good. I'll create an issue for this and https://github.com/dlrobertson/smoltcp/blob/proto-ipv4/src/lib.rs#L4-L5

whitequark · 2017-12-22T07:02:01Z

src/iface/neighbor.rs

@@ -156,6 +156,7 @@ impl<'a> Cache<'a> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


As described elsewhere.

whitequark · 2017-12-22T07:02:42Z

src/socket/raw.rs

@@ -240,6 +243,7 @@ impl<'a, 'b> RawSocket<'a, 'b> {
 }

 #[cfg(test)]
+#[cfg(feature = "proto-ipv4")]


As described elsewhere.

whitequark · 2017-12-22T09:27:33Z

I've revised required-features even further and added them upstream, so you can discard my commit here.

dlrobertson · 2017-12-22T15:15:49Z

I've revised required-features even further and added them upstream, so you can discard my commit here.

👍 I'll rebase and force push

- Users should be able to choose to use smoltcp without IPv4 support. - Add MOCK_IP_ADDR_* to wire::ip::test for testers to use.

dlrobertson · 2017-12-24T02:54:31Z

Updated. I added the mock addrs to wire::ip::test. I did not update the RawSocket or IcmpSocket tests because we should have both IPv4 and IPv6 tests for RawSockets and IcmpSocket does not support IPv6 at the moment.

BTW the test_like_travis.rb script is a lifesaver.

whitequark · 2017-12-24T11:58:48Z

Cargo.toml

 default = [
  "std", "log", # needed for `cargo test --no-default-features --features default` :/
-  "phy-raw_socket", "phy-tap_interface",
+  "phy-raw_socket", "phy-tap_interface", "proto-ipv4",


I think we should enable both in the default features, just like we enable all kinds of sockets.

whitequark · 2017-12-24T12:00:55Z

src/iface/ethernet.rs

-                _ => unreachable!()
+                #[cfg(feature = "proto-ipv6")]
+                IpRepr::Ipv6(_) => frame.set_ethertype(EthernetProtocol::Ipv6),
+                _ => { return; }


Nit: _ => return

whitequark · 2017-12-24T12:02:06Z

src/lib.rs

-#![deny(unsafe_code, unused)]
+#![deny(unsafe_code)]
+// TODO: Change this to enable deny(unused) if IPv6 or IPv4 are enabled
+#![cfg_attr(feature = "proto-ipv4", deny(unused))]


What's holding up #![deny(unused)] for IPv6?

Using it in EthernetInterface. Things like Packet and cmp are unused at the moment when only IPv6 is enabled

dlrobertson mentioned this pull request Dec 22, 2017

Add IPv6 to wire #76

Closed

3 tasks

dlrobertson force-pushed the proto-ipv4 branch 2 times, most recently from 4125efe to 0983635 Compare December 22, 2017 02:52

whitequark requested changes Dec 22, 2017

View reviewed changes

dlrobertson force-pushed the proto-ipv4 branch 7 times, most recently from 22f5dc8 to 1659155 Compare December 24, 2017 01:46

Add the proto-ipv4 feature

550bc95

- Users should be able to choose to use smoltcp without IPv4 support. - Add MOCK_IP_ADDR_* to wire::ip::test for testers to use.

dlrobertson force-pushed the proto-ipv4 branch from 1659155 to 550bc95 Compare December 24, 2017 02:53

whitequark reviewed Dec 24, 2017

View reviewed changes

whitequark added 4 commits December 24, 2017 13:12

Style. NFC.

02f0cb6

Merge branch 'master' into proto-ipv4

7191235

Merge branch 'master' into proto-ipv4

a3c6b9f

Update test matrix.

4a1dd36

whitequark approved these changes Dec 24, 2017

View reviewed changes

whitequark merged commit 507d2fe into smoltcp-rs:master Dec 24, 2017

dlrobertson deleted the proto-ipv4 branch December 24, 2017 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the proto-ipv4 feature #98

Add the proto-ipv4 feature #98

dlrobertson commented Dec 22, 2017 •

edited

whitequark Dec 22, 2017

dlrobertson Dec 23, 2017 •

edited

whitequark Dec 23, 2017

whitequark Dec 22, 2017

whitequark Dec 22, 2017

whitequark Dec 22, 2017

whitequark Dec 22, 2017

dlrobertson Dec 22, 2017

whitequark Dec 22, 2017

dlrobertson Dec 22, 2017

whitequark Dec 22, 2017

whitequark Dec 22, 2017

whitequark commented Dec 22, 2017

dlrobertson commented Dec 22, 2017

dlrobertson commented Dec 24, 2017

whitequark Dec 24, 2017

whitequark Dec 24, 2017

whitequark Dec 24, 2017

dlrobertson Dec 24, 2017

Add the proto-ipv4 feature #98

Add the proto-ipv4 feature #98

Conversation

dlrobertson commented Dec 22, 2017 • edited

Choose a reason for hiding this comment

dlrobertson Dec 23, 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

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 Dec 22, 2017

dlrobertson commented Dec 22, 2017

dlrobertson commented Dec 24, 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

dlrobertson commented Dec 22, 2017 •

edited

dlrobertson Dec 23, 2017 •

edited