I had a quick glance and I do like the idea.

Is the route table an ethernet concept or an IP concept?

Routing is layer 3 together with IP, Ethernet is layer 2 in the ISO/OSI model.

The ISO/OSI model isn't terribly relevant to real-world TCP/IP stacks; TCP is not separate from IP, and IP is not easily separate from Ethernet. Rather than considering this in abstract terms, it is best to see what effect any particular kind of separation has on your network stack, and go from that.

@Dirbaio I think your idea about separating Config and State is quite interesting. Do you think we could apply it more incrementally, i.e. first splitting the Ethernet structs into two, and only then splitting EthernetInterface?

IP is not easily separate from Ethernet.

I disagree. There's absolutely no relationship between Ethernet and IP whatsoever. You can run a lot of different protocols on top of Ethernet and there're also some commonly used IP implementations which do not run on top of Ethernet, e.g. PPP.

In this particular case the question was about routing and since routing can only work on top of a protocol like IP, there's no question for me that this does not belong in Ethernet at all which is why I brought up the ISO/OSI layers.

Rather than considering this in abstract terms, it is best to see what effect any particular kind of separation has on your network stack, and go from that.

I agree in priciple but in this particular case it seems pretty clear that routing does not belong into Ethernet.

You can run a lot of different protocols on top of Ethernet

Of course, I never implied that you can't.

there're also some commonly used IP implementations which do not run on top of Ethernet, e.g. PPP

Of course also true.

There's absolutely no relationship between Ethernet and IP whatsoever.

And this is just black-and-white thinking that obscures the real situation. If you said that IP is not strictly coupled to Ethernet in the way that TCP is coupled to IP, I would agree. However, a practical IPv4 stack, for example, cannot have a clean separation beteween the LLC sublayer and the IP layer because the concept of default gateways violates it. There is a similar issue with IPv6 NDISC. We also have DHCP, a layer 3 configuration protocol that runs on layer 4.

That is what I mean by "not easily separate": not in the sense that it is impossible to decouple IP from Ethernet (of course it is), but in the sense that real world IP implementations are a lot more messy than the model might lead you to think, and have interdepenencies between the Ethernet and IP parts, as this PR very clearly demonstrates.

The reason I bring it up is because one of the core principles of smoltcp, one that I decided on before I even wrote any code for the project, is to implement the protocols as they exist in real world and serve real users rather than adhere to a rigid model that doesn't fully apply to our system. That is something I would like to be preserved.

I agree in priciple but in this particular case it seems pretty clear that routing does not belong into Ethernet.

We are in agreement on this issue. I mostly wanted to provide historical context and explain the principles I used when designing smoltcp.

I've pushed a new version of the patches following @whitequark's suggestion. First commit splits ethernet into config+state, last commit splits these into a config+state for IP and another for Ethernet.

I find this way makes Ethernet and IP code more consistent, and hopefully makes review easier. Also, there's no more passing around ip::Config since the Ethernet processor now has access to it.

@Dirbaio I've reviewed them and I do very much like the first two commits! (Some tiny style nits, that's it.)

Regarding the third, do you think you can summarize the change in a few sentences or pargraphs? It's a massive diff and that would make it a lot easier for me to review.

I've pushed a new set of commits, splitting the last commit in two. First one splits Packet, second one splits Config, State, Processor. I'm afraid I can't think of more ways to split the last one.

Also, commit messages now have more info, summarizing the changes.

Pushed new commits.

• Implemented ndisc. (I had to add two traits, LowerDispatcher and LowerProcessor. I tried doing it with just one, but it runs into borrow issues)
• Updated tests to the new structure (they all pass without needing logic changes).

With these, this is ready for review now.

Tests are failing due to an unrelated breakage from latest nightly. I've sent a separate PR for that: #337

Thanks for splitting the changes. I like your general approach.

You've added a bunch of new pub (not pub(crate)) functions; is that intended?

Could you go through the added documentation and fix up the formatting and capitalization to be consistent with existing doc strings? This isn't terribly important but makes it nicer to read.

I'm trying to make it work by comparing with the ethernet implementation. I'd like to create an HTTP client example. As of now, ip.rs has no polling function poll. Shouldn't it have? So an example client would be almost like the one we already have on examples/client.rs. Is there a reason on why you didn't implement it, @Dirbaio?

IP is supposed to be used only internally from the actual interface structs, it's not supposed to be an interface itself. For example if you want to use a tun linux device you'd create a TunInterface that uses IP internally. poll & co methods would be duplicated, maybe there's a way around that

I'm planning to send a follow-up PR adding it.

@Dirbaio since I want to use this stack on OpenVPN, instead of creating a TunInterface impementing the Device trait that taks with a linux file descriptor, I'm thinking about creating a VirtualTunInterface that reads and writes packets from memory, so I can integrate into OpenVPN. The usage of a VirtualTunInterface would be the same as the usage of TunInterface, so I created a tun.rs file that mimics ethernet.rs. And then I created virtual_tun.rs that implements a virtual TUN device. Maybe you can implement the TunInterface later.

I believe this is what should be done. Can you take a look at my sketch? https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1

I'm willing to do the tun.rs for you, since the only difference would be the VirtualTunInterface that implements the Device trait which is minimal. But I wanted to know if that's what you have in mind.

Hey @Dirbaio, if you look at https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1, you'll see several modifications. I sucessfully made a VirtualTun device that receives packets from a queue instead of a file descriptor, and sends to a queue (queue part to be done but the device receives the packet already). I'm working with example packets to debug things since it's going to take time for me to integrate this to OpenVPN.

So what you said you would do is kinda done, I just need to refactor + reactivate the tests that I commented.

Now you'd only have to create a real Tun device instead of using my VirtualTun one.

What you think? Can you make it so we can test my modifications better?

again: code is ugly, gonna refactor a lot

Ok so I made the tun interface also. So now the project has a tap interface, a tun interface, and a virtual tun interface (reads packets from memory instead of from file descriptor).

I used Wireshark and could see the packet being sent to the tun device I created. However, I don't know how to connect this device to the world, so I can sucessfully test the HTTP client example. Right now I can see that it sends a TCP SYN but since the tun device is not connected to anything, it wont respond.

Any ideas on how to test it?

My changes: https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1

only thing missing now are mod tests that I commented + refactoring

I made the tunhttpclient example work!

sudo ip tuntap add dev tun1 mode tun user `id -un`
sudo ip link set dev tun1 up
sudo ip addr add dev tun1 local 192.168.69.0 remote 192.168.69.1
sudo iptables -t filter -I FORWARD -i tun1 -o eth0 -j ACCEPT
sudo iptables -t filter -I FORWARD -m state --state ESTABLISHED,RELATED -j ACCEPT
sudo iptables -t nat -I POSTROUTING -o eth0 -j MASQUERADE
sudo sysctl net.ipv4.ip_forward=1

./tunhttpclient 172.217.28.238 http://google.com

it sucessfully retrivies HTML from google using the tun interface :)

Great news!

tun example working and all non ethernet tests working!

https://github.com/Dirbaio/smoltcp/compare/ip-interface...lucaszanella:ip-interface?expand=1

Just need to know if process_ndisc is ethernet related or not. Anyone?

After this, I can refactor the code to take comments out and then pull request to @Dirbaio's code

Just need to know if process_ndisc is ethernet related or not. Anyone?

NDISC is the equivalent of ARP, AFAIK you don't need it for TUN interfaces (but I didn't check in depth).

thanks @whitequark, however it was not simple to remove ndisc. see:

I refactored everything. There are just 2 issues before I push to @Dirbaio's code:

Here:

https://github.com/lucaszanella/smoltcp/blob/071e416320cb2303e1c2b886550fc4169b3559f2/src/iface/tun.rs#L476

if I pass a lower that does not implement ip::LowerProcessor it won't compile because of the trait bound ip::LowerProcessor for the lower argument. This is the trait that requires process_ndisc so I left like this:

    fn process_ndisc<'frame>(&mut self, timestamp: Instant, ip_repr: Ipv6Repr,
                             repr: NdiscRepr<'frame>) -> Result<Option<ip::Packet<'frame>>> {
        self._processor.process_ndisc(timestamp, ip_repr, repr)
    }

where process_ndisc does nothing:

    fn process_ndisc<'frame>(&mut self, _timestamp: Instant, _ip_repr: Ipv6Repr,
        _repr: NdiscRepr<'frame>) -> Result<Option<ip::Packet<'frame>>> {
            Ok(None)
    }

and here:

https://github.com/lucaszanella/smoltcp/blob/071e416320cb2303e1c2b886550fc4169b3559f2/src/iface/tun.rs#L429

there's no Packet in wire/ip.rs just as there's EthernetFrame in wire/ethernet.rs

The problem is that I can't implement

impl<T: AsRef<[u8]>> PrettyPrint for Frame<T> {

in order to pretty print the packet in that line.

What should be done here? I see that there's methods to pretty print each individual packet (ipv6, igmp, udp, tcp etc) but there's no trait implementation for IP.

Hello, finally a pull requested to @Dirbaio's repo :)

https://github.com/Dirbaio/smoltcp/pull/1/files

@LucasZanella that looks great! Thanks for putting the work.

Sorry everyonw for the long delays adressing questions and review.

I've been integrating smoltcp with my changes into our product's firmware, and I've found some concerns about the current implementation.

Concern 1: binary size bloat

Right now most of the ip code is generic over the LowerProcessor trait, which means they're emitted N times as binary code, monomorphized for each type of interface in use. This monomorphization propagates to all the sockets' dispatch methods because they take a closure that uses the LowerProcessor. And all these methods are definitely not small :(

This means that if you want your firmware to support ethernet and ip devices at the same time you're going to pay quite a bit of flash space penalty. This is our case: we want to make a single device, where a single firmware supports pluggable Ethernet/Wifi modules (Ethernet medium), and cellular modules (PPP, IP medium).

You may think this is already a problem with the current code due to the Device trait: if you have two Device implementations, you're going to have two iface::Ethernet monomorphizations. That's not actually the case: you can use dynamic dispatch to the actual device code, so all the smoltcp code is only instantiated once. It's a bit convoluted because Device is not object-safe, but it's doable.

Concern 2: maintainability, code duplication

As seen in the current EthernetInterface and @LucasZanella 's TunInterface, there's quite a bit of code duplication that I don't think is easy to remove.

The tests should be refactored so all IP-specific tests are in ip module, but there's still a lot of duplication left. I really can't think of a way to remove it :(

Concern 3: the Device trait is ambiguous

You can create a iface::TunInterface with a phy::TapInterface, it compiles with zero complaints but blows up spectacularly at runtime.

Alternative solution

I think I've found a way to solve these problems:

• Add a medium method to Device, that returns Medium::Ethernet or Medium::IP.
• Rename iface::Ethernet to iface::Interface (or similar. The point is that there won't be an interface struct per medium type)
• Add checks to iface::Interface that check the medium of the device, and behave differently accordingly. For example, if medium is IP, send IP payloads to the device. If it's Ethernet, do ARP/NDISC and send Ethernet payloads to the device.

This solves the binary bloat nicely:

• If you instantiate Interface with an Ethernet-only device, the medium method will return a constant of Medium::Ethernet. This will allow the compiler to inline it and statically remove all the checks, so the resulting code should be about as performant as the current one. Same with an IP-only device.
• You can instantiate Interface with a device impl that does dynamic dispatch. This way you get only a single instantiation of all the smoltcp code, at the cost of it doing checks at runtime to see if it's dealing with an Ethernet or IP medium.

It also solves code duplication since there'll be just only one Interface struct.

I'm going to write a prototype of it. This will allow us to do code-size comparisons and decide.

I'm excited to see your solution. In general I think code duplication isn't necessarily bad but I agree that in this specific case it probably makes things harder to maintain!

Closing this in favor of #346