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

[posix] add netif TUN Ip6 Sending #2452

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

Irving-cl
Copy link
Contributor

This PR implements netif TUN Ip6 forwarding.

In specific, when a packet from the system is sent to the wpan interface, the Netif module will do the Ip6 sending. In this PR the actual sending (let NCP send the IP6 packet) hasn't been implemented and an empty implementation is used.

The PR also adds a unit test to verify IP packet sent through wpan interface can be successfully handled in netif.

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 57.77778% with 38 lines in your changes missing coverage. Please review.

Project coverage is 45.78%. Comparing base (2b41187) to head (27e9d17).
Report is 786 commits behind head on main.

Files with missing lines Patch % Lines
tests/gtest/test_netif.cpp 63.79% 20 Missing and 1 partial ⚠️
src/ncp/posix/netif.cpp 50.00% 9 Missing and 5 partials ⚠️
src/ncp/ncp_spinel.cpp 0.00% 2 Missing ⚠️
src/ncp/ncp_host.cpp 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2452       +/-   ##
===========================================
- Coverage   55.77%   45.78%   -10.00%     
===========================================
  Files          87       99       +12     
  Lines        6890    11657     +4767     
  Branches        0      846      +846     
===========================================
+ Hits         3843     5337     +1494     
- Misses       3047     6046     +2999     
- Partials        0      274      +274     

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

@Irving-cl Irving-cl force-pushed the posix_netif_wpan_tx branch 2 times, most recently from 2a5488b to a2778fa Compare August 23, 2024 06:39
@Irving-cl Irving-cl changed the title [posix] add netif TUN Ip6 Forwarding [posix] add netif TUN Ip6 Sending Aug 23, 2024
@Irving-cl Irving-cl force-pushed the posix_netif_wpan_tx branch 2 times, most recently from 345a2db to 1b5d334 Compare August 30, 2024 08:02
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.
Couple of smaller suggestions below:

if (mTunFd > aContext->mMaxFd)
{
aContext->mMaxFd = mTunFd;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR but a possible enhancement idea (for future PR)

I assume this pattern of adding certain fd to MainloopContext (updating mReadFdSet and mErrorFdSet and then also updating mMaxFd) might be done in several places in the code.

It could be helpful to add a method on MainloopContext for this. This would simplify the code and ensure we always update mMaxFd as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I'll do that in a seperate PR.

src/ncp/posix/netif.cpp Outdated Show resolved Hide resolved
@Irving-cl Irving-cl force-pushed the posix_netif_wpan_tx branch 5 times, most recently from d54b649 to f1e90f6 Compare September 1, 2024 03:54
Copy link
Contributor

@zhanglongxia zhanglongxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Irving-cl Irving-cl requested a review from jwhui September 2, 2024 08:14
@jwhui jwhui merged commit 3874d96 into openthread:main Sep 2, 2024
32 checks passed
@Irving-cl Irving-cl deleted the posix_netif_wpan_tx branch September 5, 2024 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants