search

smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.63k stars 402 forks source link

Add VLAN support #902

Open DerFetzer opened 5 months ago

DerFetzer commented 5 months ago

This PR adds basic VLAN support.

There are probably some things missing since I am not yet that familiar with the codebase. I hope that you can point me in the right direction.

If you add the VLAN config option to the loopback example it works and you can see the tags in the output as well as in the recorded pcap file.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 56.65914% with 192 lines in your changes are missing coverage. Please review.

Project coverage is 79.66%. Comparing base (4c27918) to head (5616b69).

Files Patch % Lines
src/iface/interface/mod.rs 35.38% 84 Missing :warning:
src/wire/vlan.rs 75.13% 47 Missing :warning:
src/wire/ethernet.rs 0.00% 30 Missing :warning:
src/iface/interface/ipv4.rs 6.89% 27 Missing :warning:
src/iface/interface/ethernet.rs 92.72% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #902 +/- ## ========================================== - Coverage 79.96% 79.66% -0.30% ========================================== Files 82 83 +1 Lines 28378 28783 +405 ========================================== + Hits 22693 22931 +238 - Misses 5685 5852 +167 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

DerFetzer commented 5 months ago

Ok, so first thing is to figure out how to integrate VLAN into existing tests for coverage.

thvdveld commented 5 months ago

Since src/wire/vlan/mod.rs is not that big, could you move that file to src/wire/vlan.rs? Could you also add some documentation in that file where we can find information about the frame formats? I'm not so familiar with VLAN. Thank you!

DerFetzer commented 2 months ago

@thvdveld I implemented your remarks a while back but forgot to write a new comment here. What do you think about the current state of this PR? Thanks!

thvdveld commented 2 months ago

I'm not really comfortable merging this as I'm not familiar with the VLAN/Ethernet implementation. Maybe @Dirbaio or @whitequark could take a look?

whitequark commented 2 months ago

This is a large and complex PR and I won't be reviewing it as-is; splitting it into multiple parts, starting with the wire bits, may help.