Skip to content
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

Add VLAN support #902

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add VLAN support #902

wants to merge 6 commits into from

Conversation

DerFetzer
Copy link

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.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

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

Project coverage is 79.66%. Comparing base (4c27918) to head (5616b69).
Report is 104 commits behind head on main.

Files with missing lines Patch % Lines
src/iface/interface/mod.rs 35.38% 84 Missing ⚠️
src/wire/vlan.rs 75.13% 47 Missing ⚠️
src/wire/ethernet.rs 0.00% 30 Missing ⚠️
src/iface/interface/ipv4.rs 6.89% 27 Missing ⚠️
src/iface/interface/ethernet.rs 92.72% 4 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DerFetzer
Copy link
Author

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

@thvdveld
Copy link
Contributor

thvdveld commented Feb 2, 2024

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants