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.

Sign up for GitHub

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

Jump to bottom

gnrc_ipv6_ext_frag: Initial import of IPv6 reassembly #11596

Merged
merged 3 commits into from
Sep 16, 2019
Merged

gnrc_ipv6_ext_frag: Initial import of IPv6 reassembly #11596

merged 3 commits into from
Sep 16, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 28, 2019 •
edited
Loading

Contribution description

This provides IPv6 fragment reassembly for GNRC. Fragmentation itself will come as a follow-up (#11623) to this PR, to keep the review overhead low.

Testing procedure

make -C tests/gnrc_ipv6_ext_frag all
sudo make -C tests/gnrc_ipv6_ext_frag test

Issues/PRs references

Addresses #5371 in part.

Depends on #11593 (merged).

Touches similar files as #11623 but can be merged independent.

@miri64 miri64 added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 28, 2019
@miri64 miri64 requested review from cgundogan and bergzand May 28, 2019 14:37
@miri64 miri64 changed the title Gnrc ipv6 ext/feat/ipv6 reass gnrc_ipv6_ext_frag: Initial import of IPv6 reassembly May 28, 2019
@miri64
Copy link
Member Author

miri64 commented May 28, 2019

I provided some initial unittests, but I also want to provide some tests utilizing scapy, to have actually fragments pass stack and do some weird stuff ;-).

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jun 3, 2019
@miri64
Copy link
Member Author

miri64 commented Jun 3, 2019

No longer WIP.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 3, 2019
@miri64
Copy link
Member Author

miri64 commented Jun 3, 2019

Started Murdock to see which boards have insufficient memory for the test.

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 3, 2019
@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch 2 times, most recently from 2db3e8a to 95bd09e Compare June 3, 2019 13:15
@miri64 miri64 mentioned this pull request Jun 4, 2019
Merged
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 4, 2019
@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from 95bd09e to 7612636 Compare June 4, 2019 15:15
@miri64
Copy link
Member Author

miri64 commented Jun 4, 2019

Rebased to current master. This PR now doesn't have any dependencies anymore.

@miri64 miri64 added this to the Release 2019.07 milestone Jun 4, 2019
@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from 7612636 to 11d433f Compare June 4, 2019 16:29
@miri64
Copy link
Member Author

miri64 commented Jun 4, 2019

Adapted test for "last minute" changes in #11593 (this was why murdock was failing).

@MrKevinWeiss
Copy link
Contributor

release status ping, should I change the milestone?

@miri64
Copy link
Member Author

miri64 commented Jun 13, 2019

Isn't the (soft) feature freeze still a few weeks ahead?

@MrKevinWeiss
Copy link
Contributor

ping for release

@miri64
Copy link
Member Author

miri64 commented Jun 25, 2019

@bergzand can I maybe get you away from USB for a bit? :->

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from 3c279ee to de7c330 Compare July 25, 2019 08:26
@miri64
Copy link
Member Author

miri64 commented Jul 25, 2019

Rebased to adapt for #11668.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 16, 2019
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

(Removed BOARD_BLACKLIST and BOARD_INSUFFICIENT_MEMORY (the latter temporarily to see which boards fit now) as a consequence of that.

@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

@benpicco are you alright with my changes? May I squash?

@miri64 miri64 added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before and removed CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before labels Sep 16, 2019
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

For reference: here are the test results

benpicco
benpicco reviewed Sep 16, 2019
View reviewed changes
tests/gnrc_ipv6_ext_frag/tests/01-run.py Outdated
if os.environ.get("BOARD", "") != "native":
# ethos currently can't handle the larger, rapidly sent packets by the
# IPv6 fragmentation of the Linux Kernel
print("SUCCESS")
Copy link
Contributor

Choose a reason for hiding this comment

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

Well that's cheating 😉

Can you print a warning about this, so nobody will rely on that "SUCCESS" - I guess it's there just to make automated tests happy?

Copy link
Member Author

Choose a reason for hiding this comment

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

People tend to run automated tests that hide the output anyways, but will do :D

@benpicco
Copy link
Contributor

benpicco commented Sep 16, 2019 •
edited
Loading

Yes, please squash.

I don't like printing success when no tests were run though. If this is just to keep some automated test scripts happy, can we add a warning for humans so they know nothing was actually tested?

@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

I don't like printing success when no tests were run though.

Just to be clear: the API tests in the application's C code are run. Just the interaction tests with packets are skipped.

@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

Rebased

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from eb8581f to 13ce098 Compare September 16, 2019 17:08
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

Arghs, forgot to fetch current upstream

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from 13ce098 to 2f188a0 Compare September 16, 2019 17:10
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

Now it's rebased correctly :-)

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-reass branch from 2f188a0 to 269af28 Compare September 16, 2019 17:13
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

And squashed and applied the fix for the SUCCESS message.

benpicco
benpicco approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

The code looks good and the tests run fine.
Also this will not cause any regressions as it's an optional feature.

I say let's go!

@benpicco benpicco merged commit f0187e8 into RIOT-OS:master Sep 16, 2019
@benpicco
Copy link
Contributor

Now we can continue with the second fragment of this effort :)

@miri64 miri64 deleted the gnrc_ipv6_ext/feat/ipv6-reass branch September 16, 2019 19:18
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 17, 2019
@miri64 miri64 mentioned this pull request Sep 17, 2019
Open
@miri64 miri64 mentioned this pull request Oct 4, 2019
Closed
@miri64 miri64 mentioned this pull request Nov 29, 2019
Closed
@miri64 miri64 mentioned this pull request Jan 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @MrKevinWeiss @benpicco @kb2ma